-
Notifications
You must be signed in to change notification settings - Fork 4
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/fetch cjs update #58
base: main
Are you sure you want to change the base?
Conversation
I think these changes should be marked as a major release. How I can mark it correctly? |
|
import fetch, { Response } from 'node-fetch'; | ||
import fetch, { | ||
Response, | ||
RequestInfo as RequestInfoDefault, |
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.
Why do we need RequestInfoDefault
here?
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.
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.
Pretty messy because of this hack, maybe we can fix the types at the level of our node-fetch-cjs
package? Or accept that Url
is not supported by the type (we can always call .toString() before)
@@ -45,7 +45,7 @@ | |||
"@nestjs/core": "^8.2.5", | |||
"@nestjs/testing": "^8.2.5", | |||
"@types/lodash.snakecase": "^4.1.6", | |||
"@types/node-fetch": "^2.5.12", | |||
"@types/node-fetch": "^2.6.2", |
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.
The declared dependencies are: @lido-nestjs/fetch
, node-fetch
, @types/node-fetch
, but uses the @lido-js/node-fetch-cjs
module
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.
I guess we should leave only @lido-nestjs/fetch
and try to import types from it where possible
// node-fetch v3 lose the naive polyfill URLLike | ||
// I add it in the fetch interface | ||
// The reason is TS can't handle abstraction with .toString getter |
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.
I would suggest rewriting the comment to disclose what the code does, not how it appeared here
import fetch, { Response } from 'node-fetch'; | ||
import fetch, { | ||
Response, | ||
RequestInfo as RequestInfoDefault, |
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.
Pretty messy because of this hack, maybe we can fix the types at the level of our node-fetch-cjs
package? Or accept that Url
is not supported by the type (we can always call .toString() before)
@@ -116,7 +116,7 @@ describe('Data fetching', () => { | |||
|
|||
await expect(fetchService.fetchJson(url)).rejects.toThrow(HttpException); | |||
await expect(fetchService.fetchJson(url)).rejects.toMatchObject({ | |||
message: 'Internal Server Error', | |||
// message: 'Internal Server Error', TODO: why this text showed in the prev version? |
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.
TODO is left here
href: string; | ||
} | ||
// node-fetch v3 lose the naive polyfill URLLike | ||
// I add it in the fetch interface |
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.
I would suggest rewriting the comment to disclose what the code does, not how it appeared here
We use |
Co-authored-by: avsetsin <mail@sjors.ru>
Co-authored-by: avsetsin <mail@sjors.ru>
No description provided.