Skip to content

Code optimisations to headers calls#92

Closed
simonbuerger wants to merge 3 commits intodevelopit:mainfrom
simonbuerger:code-optimisations
Closed

Code optimisations to headers calls#92
simonbuerger wants to merge 3 commits intodevelopit:mainfrom
simonbuerger:code-optimisations

Conversation

@simonbuerger
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Owner

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

@developit
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@RReverser
Copy link
Copy Markdown

Why was this never merged?

@developit
Copy link
Copy Markdown
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
Copy Markdown
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