Skip to content

The new Headers implementation should be reverted #1119

@TimothyGu

Description

@TimothyGu

Hi, I am a former maintainer of node-fetch. I noticed that #834 (merged last year) changed the implementation of the Headers class so that it is based on Proxy and URLSearchParams. It does not advertise to fix any bugs, nor does it add any features – readability improvements being the main motivation of the PR.

In the aftermath, we found that the code in #834 introduced various bugs like #917 and #1072 – the former being especially heinous as it depends on an undocumented bug in Node.js' URLSearchParams implementation. Additionally, it introduces a raft of behaviors that are incompatible with browsers' implementation of the class:

{
  const h = new Headers();
  h.get = () => { console.log('yes!'); };
  h.set = () => { console.log('yes!'); };
  h.get('abc')    // prints yes!
  h.set('abc', 1) // does not print yes!
}

console.log(new Headers() instanceof URLSearchParams);
  // prints true; should be false
console.log(Headers.prototype.values instanceof (function*(){}).constructor);
  // prints true; should be false
console.log(new Headers().values()[Symbol.toStringTag]);
  // prints Generator; should be "Headers Iterator"
  // note that Chrome implements this wrongly but Firefox and Safari both do the right thing
  // see https://crbug.com/793204
console.log(new Headers().sort);
  // prints [Function: sort]; should be undefined
console.log(Headers.prototype.get.call(1, 'content-type'));
  // prints undefined; should throw a TypeError

The two biggest readability improvements #834 brings are a) the simplification using URLSearchParams and b) generator functions. However, the first is a stretch – there isn't really anything similar between Headers and URLSearchParams implementation-wise other than the multimap interface they expose – and the second is simply against the Web IDL standard that defines how the JavaScript interface of web APIs. On the other hand, #834 starts using ES2015 Proxy, a huge hammer with lots of nuances for something that's really quite simple. (Note that I am also a current editor of the Web IDL spec, and authored much of the Node.js URLSearchParams implementation.)

It appears to me that a cost–benefit analysis would weigh against the recent rewrite of the Headers class. I ask the current maintainers to consider reverting the new implementation.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions