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

Native fetch missing methods on Headers in MS Edge #407

Closed
nhardy opened this Issue Oct 14, 2016 · 18 comments

Comments

Projects
None yet
7 participants
@nhardy

nhardy commented Oct 14, 2016

Not sure if this is entirely within the scope of this polyfill, but Microsoft's Edge's implementation of the fetch API and its corresponding Headers type are missing methods. It would be ideal if this project was able to polyfill these methods if they are not present:

Headers.entries()
Headers.getAll()
Headers.keys()
Headers.values()

@mislav

This comment has been minimized.

Member

mislav commented Oct 14, 2016

Thanks for letting us know! Agreed that we should try to polyfill the missing methods.

@jimmywarting

This comment has been minimized.

jimmywarting commented Oct 14, 2016

I did work a bit with node-fetch and thought it would be a good idea to abstract out the Headers to a separate module so it could be included anywhere and could be worked on together towards a common goal
How about just including https://www.npmjs.com/package/fetch-headers? It has support for entries, getAll, keys & values iterator and also support for 2D array new Headers([['x-foo', 'bar']])

@mislav

This comment has been minimized.

Member

mislav commented Oct 14, 2016

@jimmywarting That's not a far-fetched idea, but for now we like the convenience of having everything in a single file with no dependencies that people can easily load into their projects.

@STRML

This comment has been minimized.

STRML commented Oct 14, 2016

@jimmywarting You can get this via fetch-ponyfill's exports.

@dgraham

This comment has been minimized.

Member

dgraham commented Oct 14, 2016

The easiest fix would be to expand this check to test for entries.

if (self.fetch && self.Headers.prototype.entries) {

The missing methods are part of the Iterable interface and will likely be implemented together when Edge makes Headers conform to that interface. Firefox had a similar partial Headers implementation in the past too.

We polyfilled these methods in #295.

@dgraham

This comment has been minimized.

Member

dgraham commented Oct 15, 2016

I didn't find a report for this in the Edge issue tracker, so I reported it via Twitter.

@molant

This comment has been minimized.

molant commented Oct 15, 2016

Bug opened here

@mislav

This comment has been minimized.

Member

mislav commented Oct 17, 2016

Just a heads-up that I'm currently in the process of selectively polyfilling these methods for Edge but am running into some hurdles. Namely, Edge also doesn't support getAll, which technically isn't part of the iterable interface, but is trivial to polyfill. However, while running tests, I've noticed how some existing tests for other Headers methods fail in Edge due to it implementing the spec more precisely than us or Chrome. Therefore, we will need to adjust our test suite. I'm trying to read carefully through the spec and confirm that each of our tests is adjusted according to the spec.

@mislav

This comment has been minimized.

Member

mislav commented Oct 17, 2016

@dgraham and I decided that we are not going to polyfill these methods on Edge, because it opens a whole can of worms regarding cross-browser support due to the fact that Chrome, Firefox, and Edge implement Headers methods such as get, getAll, forEach, and iterable methods inconsistently. We decided we were not normalizing the native implemenation of Headers in any browsers. This polyfill is simply providing window.fetch to browsers that don't have it.

@mislav mislav closed this Oct 17, 2016

@mislav

This comment has been minimized.

Member

mislav commented Oct 19, 2016

/cc @nolanlawson for our discussion regarding Edge and the decision to not selectively polyfill Headers methods.

@nolanlawson

This comment has been minimized.

nolanlawson commented Oct 24, 2016

I agree it sounds like a better bet to just wait for Edge to fix these issues. Sorry for the trouble; is it possible to feature-detect and load the polyfill?

@mislav

This comment has been minimized.

Member

mislav commented Oct 24, 2016

@nolanlawson Yes:

<script>
  if (/* your feature detection here */) {
    window.fetch = undefined // have the polyfill override the native implementation
  }
</script>
<script src="./fetch.js"></script>
@nolanlawson

This comment has been minimized.

nolanlawson commented Oct 24, 2016

Yes I mean is it possible to feature detect Edge's implementation vs a full fetch() implementation. I guess by inspecting the Headers prototype?

@STRML

This comment has been minimized.

STRML commented Oct 24, 2016

Sure, that's possible that way, but you won't get https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9383422/ and the like

@nolanlawson

This comment has been minimized.

nolanlawson commented Oct 24, 2016

Fair enough, just trying to understand the impact. :) And yeah I just tested; 'entries' in Headers.prototype will suss out that issue, but obviously not the DELETE issue.

@nolanlawson

This comment has been minimized.

nolanlawson commented Oct 24, 2016

@mislav

some existing tests for other Headers methods fail in Edge due to it implementing the spec more precisely than us or Chrome

Could you provide some more details on this? If Chrome (and maybe Firefox/Safari?) are deviating from the spec, then it would be helpful for us to know. Also did you file a bug on Chrome? /cc @aliams

@nolanlawson

This comment has been minimized.

nolanlawson commented Oct 24, 2016

OK, did a bit of research and found that the Headers spec doesn't define those methods, but it is an Iterable, which does. However Iterable defines entries(), keys(), values(), and forEach(), but not getAll(), which both Firefox and Chrome implement, so I'm not sure where getAll() comes from.

@STRML

This comment has been minimized.

STRML commented Oct 24, 2016

That is indeed odd. It appears getAll() has been removed from the living spec. The MDN Link doesn't resolve.

Edit:
It appears there has been a change to get(), so that it now returns the combined value of all values for that key, rather than just the first. This change obviated getAll().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment