You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an almost complete rewrite of the Headers module (so, diff is looking crazy - it's better just to read new code completely).
The current implementation of Headers is somewhere painful to read (particularly iterator implementation). It's nowhere close to being lightweight too with inventing some wheels and avoiding modern language features.
What changes did you make? (provide an overview)
The only change to test file is the replacement of originally broken test for createHeadersLenient - it was testing for falseness properties on Headers object that was always false.
Change for other files is renaming of internal methods createHeadersLenient (to fromRawHeaders) and exportNodeCompatibleHeaders (to build-in symbol).
The approach used in this refactoring is to take advantage of the fact, that there is almost completely identical by shape built-in object in Node >= 10.0 - URLSearchParams (I do believe it was copied from Headers when introduced browser-side, so, now we can reuse it back). We extend here URLSearchParams attending minor differences in functionality (case insensitivity, parameters validation, and keys iteration) via lightweight Proxy, and major functionality differences (get and values iteration) as overloaded methods.
I'd moved exportNodeCompatibleHeaders export into Node.JS built-in symbol, so, it also now serves for everyone secret debug powerhouses - console.log(response.headers)
I've replaced createHeadersLenient and creating Headers from rawHeaders, that fixes#783.
With the use of Proxy, constructor return value and generator functions in the source is also a fun weekend reading for everybody who loves modern JavaScript 😄
Is there anything you'd like reviewers to know?
This PR purposely doesn't touch any tests to showcase that module is in equal functionality, however, there are some points to discuss:
Headers now, by side effect, exposing getAll method (as it exists on URLSearchParams) that was removed from client-side spec due to security concern and then removed from node-fetch too, then raw was introduced when we realized than on the server-side, indeed, we need to be able to get all values.
It's easy to hide this method completely (or make non-enumerable), or leave it undocumented (all other modules as Request and Response are exposing some non-spec methods and we have raw here anyway) or to document it. I do believe it's an extremely useful server-side and better fit for Cookies than raw. So @node-fetch/core probably should decide here.
See: Use case for Headers getAll whatwg/fetch#973 (comment)
There is no current test for new Headers(null) and it passes in both new and original versions of Headers. However, it throws at Chrome. Should behavior be adjusted and test added?
3. There is a subtle difference in how headers are returned concatenated - node-fetch is using , to join them while browsers use just coma. Should it be adjusted?
Before merging this, I think we should discuss the last points from the PR description.
Here are my thoughts:
I think we can expose getAll, while this was removed from the specification, I agree that it fits Node.js server environment better than raw and certainly helps in a lot of use cases.
What's the behavior in other browsers? Is it defined in the Fetch standard?
If it's something, that is described in the Fetch standard, we should adjust it.
I'm 👍🏻 for exposing and documenting getAll. We get it for free with this implementation and from the hard-voice comment Use case for Headers getAll whatwg/fetch#973 (comment) I do believe that WHATWG doesn't care about fetch standard interoperability, so, an extension that makes sense for runtime environment will not hurt.
I'm seeing raw both, badly named (because it's not exposing anything raw actually - all processed and lowercased) and not very useful if getAll will be available. So, we can remove it in 3.0.
I would be inviting @kentonv or whatever person that @cloudflare seems appropriate to join @node-fetch/core and participate in such decisions.
Both, Firefox and Chrome throws on new Headers(null)
I don't remember how this topic get here, our functionality here is equal to browsers.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
This is an almost complete rewrite of the
Headersmodule (so, diff is looking crazy - it's better just to read new code completely).The current implementation of
Headersis somewhere painful to read (particularly iterator implementation). It's nowhere close to being lightweight too with inventing some wheels and avoiding modern language features.What changes did you make? (provide an overview)
The only change to test file is the replacement of originally broken test for
createHeadersLenient- it was testing for falseness properties onHeadersobject that was always false.Change for other files is renaming of internal methods
createHeadersLenient(tofromRawHeaders) andexportNodeCompatibleHeaders(to build-in symbol).The approach used in this refactoring is to take advantage of the fact, that there is almost completely identical by shape built-in object in Node >= 10.0 -
URLSearchParams(I do believe it was copied fromHeaderswhen introduced browser-side, so, now we can reuse it back). We extend hereURLSearchParamsattending minor differences in functionality (case insensitivity, parameters validation, and keys iteration) via lightweight Proxy, and major functionality differences (getand values iteration) as overloaded methods.I'd moved
exportNodeCompatibleHeadersexport into Node.JS built-in symbol, so, it also now serves for everyone secret debug powerhouses -console.log(response.headers)I've replaced
createHeadersLenientand creatingHeadersfromrawHeaders, that fixes #783.With the use of Proxy, constructor return value and generator functions in the source is also a fun weekend reading for everybody who loves modern JavaScript 😄
Is there anything you'd like reviewers to know?
This PR purposely doesn't touch any tests to showcase that module is in equal functionality, however, there are some points to discuss:
Headers now, by side effect, exposing
getAllmethod (as it exists onURLSearchParams) that was removed from client-side spec due to security concern and then removed fromnode-fetchtoo, thenrawwas introduced when we realized than on the server-side, indeed, we need to be able to get all values.It's easy to hide this method completely (or make non-enumerable), or leave it undocumented (all other modules as Request and Response are exposing some non-spec methods and we have
rawhere anyway) or to document it. I do believe it's an extremely useful server-side and better fit for Cookies thanraw. So @node-fetch/core probably should decide here.See: Use case for Headers getAll whatwg/fetch#973 (comment)
There is no current test for
new Headers(null)and it passes in both new and original versions of Headers. However, it throws at Chrome. Should behavior be adjusted and test added?3. There is a subtle difference in how headers are returned concatenated -node-fetchis using,to join them while browsers use just coma. Should it be adjusted?