Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 39 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
10ca74e
chore: use node 20
mkurapov Apr 12, 2024
930b8c8
chore: update @types/node
mkurapov Apr 12, 2024
bc9beba
chore(ci): update actions
mkurapov Apr 12, 2024
e4ab76b
feat(open-payments): use ky as httpclient
mkurapov Apr 15, 2024
c07b378
chore(open-payments): fix request signing
mkurapov Apr 16, 2024
b5f986a
feat(open-payments): add validateResponses flag to optionally disable…
mkurapov Apr 17, 2024
cd02386
chore(open-payments): dont parse DELETE response body
mkurapov Apr 17, 2024
a96b3d8
chore(open-payments): handle DELETE body parsing
mkurapov Apr 17, 2024
11023d4
chore(open-payments): handle DELETE body parsing
mkurapov Apr 17, 2024
ff1022d
chore(open-payments): handle DELETE body parsing
mkurapov Apr 17, 2024
2867431
chore(open-payments): only try parsing DELETE body if its not a 204 r…
mkurapov Apr 17, 2024
be3f608
chore(open-payments): only try parsing DELETE body if its not a 204 r…
mkurapov Apr 17, 2024
727c37b
feat(open-payments): finalize and test requests
mkurapov Apr 18, 2024
6e74048
chore(open-payments): update nock and ky usage in jest
mkurapov Apr 18, 2024
19f2a2c
chore(open-payments): fix tests after update to ky
mkurapov Apr 18, 2024
00e02d5
Revert "feat(open-payments): add validateResponses flag to optionally…
mkurapov Apr 18, 2024
e528947
chore(open-payments): cleanup files after commit revert
mkurapov Apr 18, 2024
f69571d
chore(ci): update actions
mkurapov Apr 12, 2024
450b69e
Merge branch '458/mk/node-20' into 454/mk/use-ky
mkurapov Apr 18, 2024
4c5559c
chore: use node 20
mkurapov Apr 12, 2024
9d9f0f9
chore(ci): update actions
mkurapov Apr 12, 2024
743751d
Merge branch '458/mk/node-20' into 454/mk/use-ky
mkurapov Apr 18, 2024
6f0077c
Merge branch 'main' into 454/mk/use-ky
mkurapov Apr 18, 2024
c64919f
chore(open-payments): use dynamic import to resolve ky package
mkurapov Apr 19, 2024
8aa37a9
chore(open-payments): allow nock to patch global.fetch
mkurapov Apr 19, 2024
9285d60
chore(open-payments): allow nock to patch global.fetch in global setup
mkurapov Apr 23, 2024
d6e0d66
chore(open-payments): fix test & lint
mkurapov Apr 23, 2024
ea55707
Merge branch 'main' into 454/mk/use-ky
mkurapov Apr 23, 2024
e308f60
Merge branch 'main' into 458/mk/node-20
mkurapov Apr 23, 2024
825bde1
Merge branch '458/mk/node-20' into 454/mk/use-ky
mkurapov Apr 23, 2024
6ff4039
chore(open-payments): continue nock cleanup
mkurapov Apr 23, 2024
86d8c97
Merge branch 'main' into 454/mk/use-ky
mkurapov Apr 23, 2024
fde0c10
chore: add changeset
mkurapov Apr 23, 2024
1506ebd
chore: update jest in workspace
mkurapov Apr 24, 2024
cff2671
Revert "chore(open-payments): allow nock to patch global.fetch in glo…
mkurapov Apr 24, 2024
fa02f89
chore: update @swc/jest
mkurapov Apr 24, 2024
c4f8a42
chore(open-payments): nock cleanup
mkurapov Apr 24, 2024
a327e72
chore(open-payments): address comments
mkurapov Apr 26, 2024
6eb6ade
chore(open-payments): handling unexpected error case
mkurapov Apr 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/deploy-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v3
with:
node-version: 18
node-version: 20

- uses: pnpm/action-setup@v2
name: Install pnpm
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/env-setup/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ runs:
version: 7
- uses: actions/setup-node@v3
with:
node-version: '18'
node-version: '20'
cache: 'pnpm'
- name: Install dependencies
shell: bash
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-docs-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v3
with:
node-version: 18
node-version: 20

- uses: pnpm/action-setup@v2
name: Install pnpm
Expand Down
1 change: 1 addition & 0 deletions .nvmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
lts/iron
5 changes: 2 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,8 @@ More phone numbers: https://tel.meet/htd-eefo-ovn?hs=5
### Environment Setup

```sh
# install node 18
nvm install lts/hydrogen
nvm use lts/hydrogen
# install node from `./.nvmrc`
nvm install

# install pnpm
corepack enable
Expand Down
2 changes: 1 addition & 1 deletion jest.config.base.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

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

@mkurapov mkurapov Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
testEnvironment: 'node',
moduleDirectories: ['node_modules', './'],
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
"license": "Apache-2.0",
"repository": "https://github.com/interledger/open-payments",
"engines": {
"node": "18"
"pnpm": "^8.15.6",
"node": "20"
},
"packageManager": "[email protected]",
"scripts": {
"preinstall": "npx only-allow pnpm",
"lint": "eslint --max-warnings=0 --fix .",
Expand Down
2 changes: 1 addition & 1 deletion packages/http-signature-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"uuid": "^9.0.0"
},
"devDependencies": {
"@types/node": "^18.7.12",
"@types/node": "^20.12.7",
"@types/uuid": "^9.0.0",
"typescript": "^4.9.5"
}
Expand Down
5 changes: 4 additions & 1 deletion packages/open-payments/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ const baseConfig = require('../../jest.config.base.js')
// eslint-disable-next-line @typescript-eslint/no-var-requires
const packageName = 'open-payments'

const esModules = ['ky']

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transpile ESM ky package into CommonJS

Copy link
Contributor Author

@mkurapov mkurapov Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mkurapov mkurapov Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
6 changes: 3 additions & 3 deletions packages/open-payments/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,18 @@
"test": "jest --passWithNoTests"
},
"devDependencies": {
"@types/node": "^18.7.12",
"@types/node": "^20.12.7",
"@types/uuid": "^9.0.0",
"nock": "^13.3.0",
"nock": "14.0.0-beta.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the version with experimental support for fetch

"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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

"base64url": "^3.0.1",
"http-message-signatures": "^0.1.2",
"ky": "^1.2.3",
"pino": "^8.11.0",
"uuid": "^9.0.0"
}
Expand Down
36 changes: 19 additions & 17 deletions packages/open-payments/src/client/grant.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { HttpMethod } from '@interledger/openapi'
import { HttpMethod, ResponseValidator } from '@interledger/openapi'
import {
GrantOrTokenRequestArgs,
RouteDeps,
Expand Down Expand Up @@ -33,22 +33,24 @@ export interface GrantRoutes {
export const createGrantRoutes = (deps: GrantRouteDeps): GrantRoutes => {
const { openApi, client, ...baseDeps } = deps

const requestGrantValidator = openApi.createResponseValidator<
PendingGrant | Grant
>({
path: getASPath('/'),
method: HttpMethod.POST
})
const continueGrantValidator = openApi.createResponseValidator<
GrantContinuation | Grant
>({
path: getASPath('/continue/{id}'),
method: HttpMethod.POST
})
const cancelGrantValidator = openApi.createResponseValidator({
path: getASPath('/continue/{id}'),
method: HttpMethod.DELETE
})
let requestGrantValidator: ResponseValidator<PendingGrant | Grant>
let continueGrantValidator: ResponseValidator<GrantContinuation | Grant>
let cancelGrantValidator: ResponseValidator<void>

if (deps.validateResponses) {
requestGrantValidator = openApi.createResponseValidator({
path: getASPath('/'),
method: HttpMethod.POST
})
continueGrantValidator = openApi.createResponseValidator({
path: getASPath('/continue/{id}'),
method: HttpMethod.POST
})
cancelGrantValidator = openApi.createResponseValidator({
path: getASPath('/continue/{id}'),
method: HttpMethod.DELETE
})
}

return {
request: (
Expand Down
30 changes: 14 additions & 16 deletions packages/open-payments/src/client/incoming-payment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('incoming-payment', (): void => {
},
openApiValidators.successfulValidator
)
expect(result).toStrictEqual(incomingPayment)
expect(result).toEqual(incomingPayment)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

test('throws if incoming payment does not pass validation', async (): Promise<void> => {
Expand Down Expand Up @@ -133,7 +133,7 @@ describe('incoming-payment', (): void => {
},
openApiValidators.successfulValidator
)
expect(result).toStrictEqual(publicIncomingPayment)
expect(result).toEqual(publicIncomingPayment)
})

test('throws if incoming payment does not pass open api validation', async (): Promise<void> => {
Expand Down Expand Up @@ -176,13 +176,13 @@ describe('incoming-payment', (): void => {
const result = await createIncomingPayment(
deps,
{ url: serverAddress, accessToken },
openApiValidators.successfulValidator,
{
walletAddress,
incomingAmount,
expiresAt,
metadata
}
},
openApiValidators.successfulValidator
)

scope.done()
Expand Down Expand Up @@ -211,8 +211,8 @@ describe('incoming-payment', (): void => {
await createIncomingPayment(
deps,
{ url: serverAddress, accessToken },
openApiValidators.successfulValidator,
{ walletAddress }
{ walletAddress },
openApiValidators.successfulValidator
)
} catch (error) {
assert.ok(error instanceof OpenPaymentsClientError)
Expand All @@ -237,8 +237,8 @@ describe('incoming-payment', (): void => {
createIncomingPayment(
deps,
{ url: serverAddress, accessToken },
openApiValidators.failedValidator,
{ walletAddress }
{ walletAddress },
openApiValidators.failedValidator
)
).rejects.toThrowError()
scope.done()
Expand Down Expand Up @@ -266,7 +266,7 @@ describe('incoming-payment', (): void => {

scope.done()

expect(result).toStrictEqual(incomingPayment)
expect(result).toEqual(incomingPayment)
})

test('throws if the incoming payment does not pass validation', async (): Promise<void> => {
Expand Down Expand Up @@ -364,7 +364,7 @@ describe('incoming-payment', (): void => {
}
)

expect(result).toStrictEqual(incomingPaymentPaginationResult)
expect(result).toEqual(incomingPaymentPaginationResult)
scope.done()
}
)
Expand Down Expand Up @@ -407,7 +407,7 @@ describe('incoming-payment', (): void => {
}
)

expect(result).toStrictEqual(incomingPaymentPaginationResult)
expect(result).toEqual(incomingPaymentPaginationResult)
scope.done()
}
)
Expand Down Expand Up @@ -498,9 +498,7 @@ describe('incoming-payment', (): void => {
completed: true
})

expect(validateIncomingPayment(incomingPayment)).toStrictEqual(
incomingPayment
)
expect(validateIncomingPayment(incomingPayment)).toEqual(incomingPayment)
})

test('throws if receiving and incoming amount asset scales are different', async (): Promise<void> => {
Expand Down Expand Up @@ -557,7 +555,7 @@ describe('incoming-payment', (): void => {
}
})

expect(validateCreatedIncomingPayment(incomingPayment)).toStrictEqual(
expect(validateCreatedIncomingPayment(incomingPayment)).toEqual(
incomingPayment
)
})
Expand Down Expand Up @@ -593,7 +591,7 @@ describe('incoming-payment', (): void => {
completed: true
})

expect(validateCompletedIncomingPayment(incomingPayment)).toStrictEqual(
expect(validateCompletedIncomingPayment(incomingPayment)).toEqual(
incomingPayment
)
})
Expand Down
Loading
Loading