feat(open-payments): replace axios with ky #461

mkurapov · 2024-04-18T16:41:47Z

Changes proposed in this pull request

First part of making the open payments SDK usable in the web extension manifest V3:
Replace axios with a wrapper for native fetch (ky). This is due to the fact that in the web extension we cannot use XMLHttpRequest that axios uses, but native fetch is (of course) supported.

Context

Progress toward #454

changeset-bot · 2024-04-18T16:41:51Z

🦋 Changeset detected

Latest commit: 6eb6ade

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@interledger/open-payments	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-04-18T16:42:02Z

✅ Deploy Preview for openpayments-preview canceled.

Name	Link
🔨 Latest commit	`6eb6ade`
🔍 Latest deploy log	https://app.netlify.com/sites/openpayments-preview/deploys/662fc485e063900008a70b46

… openapi schema validation

…esponse

… disable openapi schema validation" This reverts commit b5f986a. # Conflicts: # packages/open-payments/src/client/requests.test.ts # packages/open-payments/src/client/requests.ts

mkurapov · 2024-04-18T16:53:33Z

jest.config.base.js

@@ -2,7 +2,7 @@

 module.exports = {
  transform: {
-    '^.+\\.tsx?$': ['@swc/jest']
+    '^.+\\.(t|j)sx?$': ['@swc/jest']


this is so we transpile JS files as well, particularly for the files we need to transform from ESM to CommonJS in transformIgnorePatterns -> node_modules/ky

mkurapov · 2024-04-18T16:53:39Z

packages/open-payments/package.json

    "openapi-typescript": "^4.5.0",
    "typescript": "^4.9.5"
  },
  "dependencies": {
    "@interledger/http-signature-utils": "workspace:2.0.2",
    "@interledger/openapi": "workspace:2.0.1",
-    "axios": "^1.6.0",


mkurapov · 2024-04-18T16:55:02Z

packages/open-payments/package.json

@@ -32,16 +32,16 @@
  "devDependencies": {
    "@types/node": "^20.12.7",
    "@types/uuid": "^9.0.0",
-    "nock": "^13.3.0",
+    "nock": "14.0.0-beta.5",


this is the version with experimental support for fetch

# Conflicts: # .github/workflows/env-setup/action.yaml

…bal setup" This reverts commit 9285d60.

mkurapov · 2024-04-24T14:45:11Z

package.json

@@ -24,14 +24,15 @@
    "@changesets/cli": "^2.26.1",
    "@commitlint/cli": "^17.4.4",
    "@commitlint/config-conventional": "^17.4.4",
-    "@swc/jest": "^0.2.24",
-    "@types/jest": "^29.4.4",
+    "@swc/core": "^1.5.0",


it's recommended to add @swc/core explicitly when using @swc/jest: https://swc.rs/docs/usage/jest

mkurapov · 2024-04-24T14:45:26Z

packages/open-payments/jest.config.js

@@ -13,5 +15,6 @@ module.exports = {
  modulePaths: [`<rootDir>/packages/${packageName}/src/`],
  id: packageName,
  displayName: packageName,
-  rootDir: '../..'
+  rootDir: '../..',
+  transformIgnorePatterns: [`node_modules/(?!.pnpm|${esModules.join('|')})`]


transpile ESM ky package into CommonJS

This also means we will need to add the same to the rafiki tests (everywhere where we use the @interledger/open-payments library, since rafiki also uses CJS only

How does this relate to the problem w/ the new ESM version of adonis which we couldnt get working in jest?

interledger/rafiki#2334 (comment)

Is this going to be a problem in rafiki for the same reason? Does it clarify that problem at all?

I think what will end up working is doing the same in Rafiki and by adding the Adonis library to the list of esmModules. Have yet to try, but it follows the same principle

mkurapov · 2024-04-24T14:46:16Z

packages/open-payments/src/client/requests.ts

+export const createHttpClient = async (
+  args: CreateHttpClientArgs
+): Promise<HttpClient> => {
+  const { default: ky } = await import('ky')


ky is ESM only

mkurapov · 2024-04-24T14:46:44Z

packages/open-payments/src/client/requests.ts

-      status,
-      body: data || undefined
+      status: response.status,
+      body: undefined


we always expect 204 in our requests which always come w/o body

mkurapov · 2024-04-25T12:44:42Z

tsconfig.build.json

@@ -43,7 +43,7 @@
    // "noPropertyAccessFromIndexSignature": true,  /* Require undeclared properties from index signatures to use element accesses. */

    /* Module Resolution Options */
-    "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */,
+    "moduleResolution": "NodeNext" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */,


this is so we properly transpile dynamic import:
https://www.typescriptlang.org/tsconfig/#moduleResolution
(without changing this, we incorrectly compile the dynamic import back to require - which ends up breaking for an ESM only module like ky)

BlairCurrey · 2024-04-25T18:54:23Z

packages/open-payments/src/client/incoming-payment.test.ts

@@ -64,7 +66,7 @@ describe('incoming-payment', (): void => {
        },
        openApiValidators.successfulValidator
      )
-      expect(result).toStrictEqual(incomingPayment)
+      expect(result).toEqual(incomingPayment)


Can you elaborate on why this was necessary? I changed back to toStrictEqual and it fails with serializes to the same string. Something that changed with nock? or?

I thought this was odd as well, but I think it's the difference between how a response is returned in axios vs fetch. It's possible the order in which the object keys are returned are different?

Someone has an unaswered question related exactly to this: https://stackoverflow.com/questions/76211440/why-does-jests-tostrictequal-fail-to-match-json-responses-made-using-the-fetch

huh, yeah that is interesting. Wonder what the difference is. Seems innocent enough though.

BlairCurrey · 2024-04-26T13:10:56Z

packages/open-payments/src/client/requests.ts

+  deps.logger.error(
+    { status: errorStatus, errorDescription, url, requestType },
+    errorMessage
+  )


This is probably quite the edge case, but errorStatus and errorDescription could be undefined if the error is not a validation error, HTTPError, or Error. Just wondering if we can handle that better such as logging the original error or something. Also thought about defaulting these values to something but in that case we'd still be left to wonder what the original "error" was.

Yep, seems quite unlikely, but updated to log in any case 👍

packages/open-payments/src/client/requests.ts

BlairCurrey

Beyond the couple suggestions and questions I left, it looks good with the caveat that I couldn't get it to work when plugging this into Rafiki's integration tests. Although perhaps you have more details or already figured it out. When running it I got:

console.error
    TypeError: A dynamic import callback was invoked without --experimental-vm-modules

You mentioned the jest config would need the same transformIgnorePatterns and transform settings as here but after adding those I still see an error:

ReferenceError: esModules is not defined

mkurapov · 2024-04-29T14:40:50Z

(Spoke in slack, but adding for visibility)

is how we can support this change of the open payments library in rafiki tests: interledger/rafiki@main...mk/op-client-ky-tests

mkurapov added 5 commits April 12, 2024 11:55

chore: use node 20

10ca74e

chore: update @types/node

930b8c8

chore(ci): update actions

bc9beba

feat(open-payments): use ky as httpclient

e4ab76b

chore(open-payments): fix request signing

c07b378

mkurapov changed the base branch from main to 458/mk/node-20 April 18, 2024 16:42

mkurapov added 10 commits April 18, 2024 18:47

feat(open-payments): add validateResponses flag to optionally disable…

b5f986a

… openapi schema validation

chore(open-payments): dont parse DELETE response body

cd02386

chore(open-payments): handle DELETE body parsing

a96b3d8

chore(open-payments): handle DELETE body parsing

11023d4

chore(open-payments): handle DELETE body parsing

ff1022d

chore(open-payments): only try parsing DELETE body if its not a 204 r…

2867431

…esponse

chore(open-payments): only try parsing DELETE body if its not a 204 r…

be3f608

…esponse

feat(open-payments): finalize and test requests

727c37b

chore(open-payments): update nock and ky usage in jest

6e74048

chore(open-payments): fix tests after update to ky

19f2a2c

mkurapov force-pushed the 454/mk/use-ky branch from 203ccbd to 19f2a2c Compare April 18, 2024 16:48

Revert "feat(open-payments): add validateResponses flag to optionally…

00e02d5

… disable openapi schema validation" This reverts commit b5f986a. # Conflicts: # packages/open-payments/src/client/requests.test.ts # packages/open-payments/src/client/requests.ts

mkurapov commented Apr 18, 2024

View reviewed changes

chore(open-payments): cleanup files after commit revert

e528947

mkurapov changed the title ~~454/mk/use ky~~ feat(open-payments): replace axios with ky Apr 18, 2024

chore(ci): update actions

f69571d

mkurapov force-pushed the 458/mk/node-20 branch from bc9beba to f69571d Compare April 18, 2024 16:59

mkurapov added 3 commits April 18, 2024 19:02

Merge branch '458/mk/node-20' into 454/mk/use-ky

450b69e

# Conflicts: # .github/workflows/env-setup/action.yaml

chore: use node 20

4c5559c

chore(ci): update actions

9d9f0f9

Base automatically changed from 458/mk/node-20 to main April 23, 2024 14:43

mkurapov added 6 commits April 23, 2024 17:47

Merge branch 'main' into 454/mk/use-ky

86d8c97

chore: add changeset

fde0c10

chore: update jest in workspace

1506ebd

Revert "chore(open-payments): allow nock to patch global.fetch in glo…

cff2671

…bal setup" This reverts commit 9285d60.

chore: update @swc/jest

fa02f89

chore(open-payments): nock cleanup

c4f8a42

mkurapov commented Apr 24, 2024

View reviewed changes

mkurapov marked this pull request as ready for review April 24, 2024 14:48

mkurapov requested review from raducristianpopa and BlairCurrey April 24, 2024 14:48

mkurapov commented Apr 25, 2024

View reviewed changes

BlairCurrey reviewed Apr 25, 2024

View reviewed changes

BlairCurrey reviewed Apr 26, 2024

View reviewed changes

packages/open-payments/src/client/requests.ts Outdated Show resolved Hide resolved

BlairCurrey reviewed Apr 26, 2024

View reviewed changes

packages/open-payments/src/client/requests.ts Outdated Show resolved Hide resolved

BlairCurrey reviewed Apr 26, 2024

View reviewed changes

packages/open-payments/src/client/requests.ts Outdated Show resolved Hide resolved

BlairCurrey reviewed Apr 26, 2024

View reviewed changes

chore(open-payments): address comments

a327e72

mkurapov linked an issue Apr 29, 2024 that may be closed by this pull request

Prepare @interledger/open-payments lib for use with manifest V3 #454

Closed

3 tasks

BlairCurrey approved these changes Apr 29, 2024

View reviewed changes

chore(open-payments): handling unexpected error case

6eb6ade

raducristianpopa approved these changes Apr 29, 2024

View reviewed changes

mkurapov merged commit 8950c21 into main Apr 29, 2024
10 checks passed

mkurapov deleted the 454/mk/use-ky branch April 29, 2024 19:46

mkurapov mentioned this pull request May 14, 2024

Replace axios interledger/rafiki#1036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(open-payments): replace axios with ky #461

feat(open-payments): replace axios with ky #461

mkurapov commented Apr 18, 2024 •

edited

Loading

changeset-bot bot commented Apr 18, 2024 •

edited

Loading

netlify bot commented Apr 18, 2024 •

edited

Loading

mkurapov Apr 18, 2024 •

edited

Loading

mkurapov Apr 18, 2024

mkurapov Apr 18, 2024

mkurapov Apr 24, 2024

mkurapov Apr 24, 2024

mkurapov Apr 25, 2024 •

edited

Loading

BlairCurrey Apr 25, 2024

mkurapov Apr 26, 2024 •

edited

Loading

mkurapov Apr 24, 2024

mkurapov Apr 24, 2024

mkurapov Apr 25, 2024 •

edited

Loading

BlairCurrey Apr 25, 2024

mkurapov Apr 26, 2024

BlairCurrey Apr 26, 2024

BlairCurrey Apr 26, 2024 •

edited

Loading

mkurapov Apr 29, 2024

BlairCurrey left a comment

mkurapov commented Apr 29, 2024

feat(open-payments): replace axios with ky #461

feat(open-payments): replace axios with ky #461

Conversation

mkurapov commented Apr 18, 2024 • edited Loading

Changes proposed in this pull request

Context

changeset-bot bot commented Apr 18, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Apr 18, 2024 • edited Loading

✅ Deploy Preview for openpayments-preview canceled.

mkurapov Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey left a comment

Choose a reason for hiding this comment

mkurapov commented Apr 29, 2024

mkurapov commented Apr 18, 2024 •

edited

Loading

changeset-bot bot commented Apr 18, 2024 •

edited

Loading

netlify bot commented Apr 18, 2024 •

edited

Loading

mkurapov Apr 18, 2024 •

edited

Loading

mkurapov Apr 25, 2024 •

edited

Loading

mkurapov Apr 26, 2024 •

edited

Loading

mkurapov Apr 25, 2024 •

edited

Loading

BlairCurrey Apr 26, 2024 •

edited

Loading