Sitelet https://web.archive.org/web/20201201154517/https://github.com/developit/unfetch/pull/92
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

Open
wants to merge 3 commits into
base: master
from

Conversation

@simonbuerger
Copy link

@simonbuerger simonbuerger commented Sep 20, 2018

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
Author

@simonbuerger simonbuerger commented Sep 20, 2018

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

@developit developit commented Sep 20, 2018

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

@developit developit commented Sep 20, 2018

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
Author

@simonbuerger simonbuerger commented Sep 20, 2018

@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
Author

@simonbuerger 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
Author

@simonbuerger 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 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
Author

@simonbuerger simonbuerger commented Oct 9, 2018

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
Author

@simonbuerger simonbuerger commented Jan 15, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.