Skip to content
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

Code optimisations to headers calls #92

Closed
wants to merge 3 commits into from

Conversation

simonbuerger
Copy link
Contributor

A few things to unpack here

As far as I can tell we only need the header keys, we don't actually need to construct the entries and headers objects with combined values, as the method on XMLHttpRequest getResponseHeader already does all that. It returns combined values and is case-insensitive. I guess the only risk would be if some older browser hasn't correctly implemented the original spec for it? Don't know if that's the reason you went this route.

Also according to spec header keys shouldn't contain duplicates. Ditto for header entries. So I'm checking before pushing new keys to the array

Directly using request.getResponseHeader for the header entries call and has and get checks. Kind of makes this difficult to mock/test as we're relying on the underlying XMLHttpRequest implementations.

Directly use case insensitive request.getResponseHeader for the header values checks
@simonbuerger
Copy link
Contributor Author

Build "unfetch" to dist:
        464 B: unfetch.js
        464 B: unfetch.mjs
        533 B: unfetch.umd.js
Build "unfetch" to polyfill:
        466 B: index.js

@developit
Copy link
Owner

ooooh you're so right! this is a great optimization.

src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Show resolved Hide resolved
@developit
Copy link
Owner

Alright I think I found an issue with this approach:

getResponseHeader(key) is awesome because it gives us case-insensitive headers.{get,has}(), but it might cause problems with multiple headers of the same name:

// for an HTTP response like this:
//   200 OK
//   Foo: one
//   Foo: two

r.getResponseHeader('foo') === 'one, two'

In this case I'm not sure how we could feasibly separate the header values. The spec says they're always concatenated using ", ", but header values can contain that sequence of characters too.

@simonbuerger
Copy link
Contributor Author

@developit wondering why we would need to separate the values? The previous approach would return ['foo', 'foo'] for keys(), [['foo', 'one, two'],['foo', 'one, two']] for entries(), 'one, two' for the get() and true for the has(). This should do one better in not duping the entries or keys, but otherwise behave the same? Or am I missing something

@simonbuerger
Copy link
Contributor Author

simonbuerger commented Sep 20, 2018

@developit Actually I see the old way didn't combine the values for the entries mthod. That seems to be incorrect.

If you run the below code:

var h = new Headers()
h.append('Foo', 'bar')
h.append('Foo', 'baz')

var entries = [];

for (var i of h.entries()) {
  entries.push(i)
}

console.log(entries) // => [['foo', 'bar, baz']]

@simonbuerger
Copy link
Contributor Author

simonbuerger commented Sep 20, 2018

@developit I think the note here https://fetch.spec.whatwg.org/#headers-class is relevant

I've done a bit more research and previously there was a getAll() method which returned non combined name value pairs. This was removed from the spec and now all iterators and get() return the combined values. Only the set-cookie header actually causes problems, but since it's not exposed to the fetch response that was deemed acceptable. This has caused some issues for libraries like node-fetch, and they worked around it by using the non standard raw() method which returns combined values, but as an array.

node-fetch/node-fetch#251 (comment)

@developit
Copy link
Owner

developit commented Oct 8, 2018

Thanks for digging so deeply into this one! I think I was remembering the old .get() behavior, and forgot that concat was already what we return for get(). I think we're probably good to merge this.

One huge perk of the size drop you've managed here is that it gives us room to explore supporting Headers, Request and Response. There are some oddities associated with exporting these in the ponyfill version, but for the polyfill I think we might be able to fit them into the newfound 50b.

btw - I just spent a week around Cape Town. Nice place!

@simonbuerger
Copy link
Contributor Author

Glad to be able to contribute 😊

And thanks, I rather like it here too!

@simonbuerger simonbuerger mentioned this pull request Oct 12, 2018
@simonbuerger
Copy link
Contributor Author

@developit let me know if you need me to do anything to move this along

@RReverser
Copy link

Why was this never merged?

@developit
Copy link
Owner

@RReverser because I'm a bad person!

developit added a commit that referenced this pull request Dec 30, 2022
Co-authored-by: Simon Buerger <[email protected]>
Co-authored-by: Jason Miller <[email protected]>
@developit
Copy link
Owner

Alrighty, I merged this manually (had to rewrite the history). Sorry for the (checks watch) 5 year delay!

@developit developit closed this Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants