Feat/fetch cjs update #58
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.
Why do we need RequestInfoDefault here?
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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.
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.
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.
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? | |||
| href: string; | ||
| } | ||
| // node-fetch v3 lose the naive polyfill URLLike | ||
| // I add it in the fetch interface |
There was a problem hiding this comment.
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.