-
Notifications
You must be signed in to change notification settings - Fork 34
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
Changes from all commits
10ca74e
930b8c8
bc9beba
e4ab76b
c07b378
b5f986a
cd02386
a96b3d8
11023d4
ff1022d
2867431
be3f608
727c37b
6e74048
19f2a2c
00e02d5
e528947
f69571d
450b69e
4c5559c
9d9f0f9
743751d
6f0077c
c64919f
8aa37a9
9285d60
d6e0d66
ea55707
e308f60
825bde1
6ff4039
86d8c97
fde0c10
1506ebd
cff2671
fa02f89
c4f8a42
a327e72
6eb6ade
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@interledger/open-payments': minor | ||
--- | ||
|
||
Replace axios with [ky](https://github.com/sindresorhus/ky) (a wrapper around native fetch) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's recommended to add |
||
"@swc/jest": "^0.2.36", | ||
"@types/jest": "^29.5.12", | ||
"@typescript-eslint/eslint-plugin": "^5.55.0", | ||
"@typescript-eslint/parser": "^5.55.0", | ||
"eslint": "^8.36.0", | ||
"eslint-config-prettier": "^8.7.0", | ||
"husky": "^8.0.3", | ||
"jest": "^29.5.0", | ||
"jest": "^29.7.0", | ||
"prettier": "^2.8.4", | ||
"ts-node-dev": "^2.0.0", | ||
"typescript": "^4.9.5" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -13,5 +15,6 @@ module.exports = { | |
modulePaths: [`<rootDir>/packages/${packageName}/src/`], | ||
id: packageName, | ||
displayName: packageName, | ||
rootDir: '../..' | ||
rootDir: '../..', | ||
transformIgnorePatterns: [`node_modules/(?!.pnpm|${esModules.join('|')})`] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. transpile ESM ky package into CommonJS There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import { getRSPath } from '../types' | |
import { OpenPaymentsClientError } from './error' | ||
import assert from 'assert' | ||
import { getResourceServerOpenAPI } from '../openapi' | ||
import { BaseDeps } from '.' | ||
|
||
jest.mock('./requests', () => { | ||
return { | ||
|
@@ -37,12 +38,13 @@ jest.mock('./requests', () => { | |
|
||
describe('incoming-payment', (): void => { | ||
let openApi: OpenAPI | ||
let deps: BaseDeps | ||
|
||
beforeAll(async () => { | ||
openApi = await getResourceServerOpenAPI() | ||
deps = await createTestDeps() | ||
}) | ||
|
||
const deps = createTestDeps() | ||
const walletAddress = 'http://localhost:1000/alice/.well-known/pay' | ||
const serverAddress = 'http://localhost:1000' | ||
const accessToken = 'accessToken' | ||
|
@@ -64,7 +66,7 @@ describe('incoming-payment', (): void => { | |
}, | ||
openApiValidators.successfulValidator | ||
) | ||
expect(result).toStrictEqual(incomingPayment) | ||
expect(result).toEqual(incomingPayment) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on why this was necessary? I changed back to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> => { | ||
|
@@ -133,7 +135,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> => { | ||
|
@@ -266,7 +268,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> => { | ||
|
@@ -364,7 +366,7 @@ describe('incoming-payment', (): void => { | |
} | ||
) | ||
|
||
expect(result).toStrictEqual(incomingPaymentPaginationResult) | ||
expect(result).toEqual(incomingPaymentPaginationResult) | ||
scope.done() | ||
} | ||
) | ||
|
@@ -407,7 +409,7 @@ describe('incoming-payment', (): void => { | |
} | ||
) | ||
|
||
expect(result).toStrictEqual(incomingPaymentPaginationResult) | ||
expect(result).toEqual(incomingPaymentPaginationResult) | ||
scope.done() | ||
} | ||
) | ||
|
@@ -498,9 +500,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> => { | ||
|
@@ -557,7 +557,7 @@ describe('incoming-payment', (): void => { | |
} | ||
}) | ||
|
||
expect(validateCreatedIncomingPayment(incomingPayment)).toStrictEqual( | ||
expect(validateCreatedIncomingPayment(incomingPayment)).toEqual( | ||
incomingPayment | ||
) | ||
}) | ||
|
@@ -593,7 +593,7 @@ describe('incoming-payment', (): void => { | |
completed: true | ||
}) | ||
|
||
expect(validateCompletedIncomingPayment(incomingPayment)).toStrictEqual( | ||
expect(validateCompletedIncomingPayment(incomingPayment)).toEqual( | ||
incomingPayment | ||
) | ||
}) | ||
|
There was a problem hiding this comment.
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