fix: proper fetch-spec related input handling #86
Conversation
| @@ -6,7 +6,7 @@ | |||
| "types": "src/index.ts", | |||
| "scripts": { | |||
| "test": "nyc ava", | |||
| "lint": "eslint . --ext .js,.ts -f codeframe && tsc --noEmit", | |||
There was a problem hiding this comment.
I have removed JS files as if you have compiled files around it is not passing linting
There was a problem hiding this comment.
Thank you for the fix! I think this improvement would be very nice to add. I'd love to help get it merged. There are a couple of changes that I'm curiuos about.
If you don't mind, can you @ message me so I get a direct notification again? It's too easy for things to get lost
| @@ -129,24 +129,8 @@ test('handles text body with content-type init options', async (test) => { | |||
| test.deepEqual(axiosBody.headers['content-type'], expectedBody.headers['content-type']); | |||
| }); | |||
|
|
|||
| test('handles json body init options', async (test) => { | |||
There was a problem hiding this comment.
Can this test be added back, or did it stop working?
There was a problem hiding this comment.
Well, thanks to proper Fetch function types definition on line 7, the types follow the Fetch spec, which does not allow JSON in body. The test is working, because underneath axios that support JSON body converts it and send it, but this is not use case supported by fetch.
I can make it working using @ts-ignore as otherwise there are types errors. Let me know what you want.
There was a problem hiding this comment.
Dropping support for JSON in body is scary to me. Since the tests run both node-fetch and also axios-fetch based requests. Does that mean that node-fetch already supported the non-standard "JSON in body" approach? If so, I'd prefer to avoid breaking that feature, I'm almost positive that some code already depends on it working.
There was a problem hiding this comment.
Sooo 😅 I wanted to implement it based on your request, but...
I have found out that actually, that use-case does not work even with the current master version 😉 The reason why it seems it works is that the test is written wrongly. I have printed out the body that nock receives and as per my suspicion I got:
Body: [object Object]
Body: [object Object]
And that is most probably because you use the String(init.body) and the tests is passing because you don't actually assert that the received body is the same as the sent one, but that the received bodies from both fetch and axios-fetch are the same which they are... [object Object].
So I think it is safe to "break" this feature because nobody could have used it 😉
There was a problem hiding this comment.
Oh wow. That's a great insight! Sorry I missed this last comment. I just did a pass through my GitHub notifications and saw it. Let me check how close this is to mergable.
| @@ -129,24 +129,8 @@ test('handles text body with content-type init options', async (test) => { | |||
| test.deepEqual(axiosBody.headers['content-type'], expectedBody.headers['content-type']); | |||
| }); | |||
|
|
|||
| test('handles json body init options', async (test) => { | |||
There was a problem hiding this comment.
Well, thanks to proper Fetch function types definition on line 7, the types follow the Fetch spec, which does not allow JSON in body. The test is working, because underneath axios that support JSON body converts it and send it, but this is not use case supported by fetch.
I can make it working using @ts-ignore as otherwise there are types errors. Let me know what you want.
|
@mdlavin I have fixed the |
| @@ -2,9 +2,11 @@ import { Response, Request, Headers as FetchHeaders, RequestInfo, RequestInit } | |||
| import { AxiosInstance, AxiosRequestConfig } from './types'; | |||
| import FormData from 'form-data'; | |||
|
|
|||
| export type AxiosTransformer = (config: AxiosRequestConfig, input: RequestInfo, init?: RequestInit) => AxiosRequestConfig; | |||
| export type FetchInit = RequestInit & { extra?: any } | |||
There was a problem hiding this comment.
The idea was that any extra options (a superset of RequestInit) would be passed to the transformer, not just extra
There was a problem hiding this comment.
It's be fine if you did some hacky typing in the test code to make it work
| "pretest": "yarn lint", | ||
| "coverage": "nyc report --reporter=text-lcov > ./.nyc_output/lcov.info", | ||
| "prepublishOnly": "yarn tsc" |
There was a problem hiding this comment.
This rename allows installation of this branch (and generally the package) from GitHub. If you don't want it, I can revert that before merge, but I needed it for our project to be used as a workaround until this PR is merged and released.
See: https://docs.npmjs.com/cli/v7/using-npm/scripts#life-cycle-scripts
There was a problem hiding this comment.
When the tests all pass then there is a pre-release version that is published to NPM.
Closes #83
This PR improves spec compliance of this library with accepting
Requestinstance as input and several other small improvements (see comments).Also, I am not sure how
form-datapackage works with axios as axios should not accept anything else than DOM instance ofform-dataas you can see here, but it indeed works which leaves me bit buffled. But I would advise against using `form-data as it is non-compliant with the spec.Btw. also I would recommend separating the compiled JS files into build directory as for example if you build the project and then run tests it uses the builder sources and not the TypeScript one.