Headers polyfill is missing properties and incorrectly adds getAll

[kentcdodds]

What version of Remix are you using?

1.19.2

Are all your remix dependencies & dev-dependencies using the same version?

• Yes

Steps to Reproduce

const headers = new Headers()
headers.getSetCookie // not defined, should be.

The typings aren't there for it in TypeScript either. I've opened an issue about that: microsoft/TypeScript#55270 (this has been fixed)

Also, there are properties in URLSearchParams that aren't on Headers:

• getAll
• sort
• size
• toString (should just be [object Headers]

Expected Behavior

Headers should follow the standard

Actual Behavior

It is missing a property and adds some properties that aren't in the spec.

[haines]

With TypeScript v5.2.2, this causes type checking to fail

node_modules/@remix-run/web-fetch/dist/src/headers.d.ts:19:22 - error TS2420: Class 'import("/Users/andrew/cerbos/spitfire/ui/node_modules/@remix-run/web-fetch/dist/src/headers").default' incorrectly implements interface 'Headers'.
  Property 'getSetCookie' is missing in type 'import("/Users/andrew/cerbos/spitfire/ui/node_modules/@remix-run/web-fetch/dist/src/headers").default' but required in type 'Headers'.

19 export default class Headers extends URLSearchParams implements globalThis.Headers {
                        ~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:13447:5
    13447     getSetCookie(): string[];
              ~~~~~~~~~~~~~~~~~~~~~~~~~
    'getSetCookie' is declared here.

[kentcdodds]

The types have been updated in TypeScript

[kentcdodds]

I'm confused why this was removed from v2. This will technically be a breaking change bug fix so it would be better to make this happen as a part of v2.

[MichaelDeBoey]

@brophdawg11 This also breaks all (v2) TS templates

[brophdawg11]

This breaks all v1 templates too though, right? It's not a new issue in v2...

I think we'll need to get @mjackson to weigh in on this, but the way I've been thinking about it, this would fall into the grey area of one of those "bug fix not breaking change" fixes, since we'd be fixing the spec-incorrect aspect of our current Headers polyfill. It would be a major version update to @remix-run/web-fetch but from a Remix standpoint where we expect the globally available fetch to be spec-compliant (wherever it comes from), it feels like a bug fix? Undoubtably, someone will be relying on the spec-incorrect behavior (i.e., headers.getAll()), but that's something I igured we could be explicit about in the release notes.

The reason I removed it from the v2 board is just because it's sort of a total rewrite of our Headers polyfill. We forked/inherited that from web-std-io as extends URLSearchParams which, as pointed out, is just flat out wrong. We were nearing what we thought was a final v2 prerelease and this type of major rewrite felt a bit out of scope to block the release.

I'm happy to be convinced otherwise (or more importantly, convince @mjackson or @ryanflorence) that this is a true breaking change and a rewrite needs to hold up a stable v2 release.

If anyone knows of a spec-compliant Headers implementation, maybe that could reduce the LOE to get it rewritten...

https://github.com/mswjs/headers-polyfill maybe?

[kentcdodds]

Thanks for explaining that!

Paging @kettanaito who would probably be able to provide some insight here as the headers polyfill is his project.

[kettanaito]

To my best knowledge, headers-polyfill should be fully compatible with the latest state of the Headers class in JavaScript. We've just recently added a new getSetCookie method and are actively maintaining the project otherwise. We don't re-implement everything from scratch but rather keep a simple internal Map of headers to ease header references and support case insensitivity, and employ third-party packages, like set-cookie-parser for header parsing heavy-lifting because we'd rather use a battle-tested third-party.

If you are looking for a complete from-scratch zero-dependency polyfill of Headerrs, then you're better off with something else. If you're looking for a drop-in replacement for Headers, then this is what we've been using in MSW for years now.

That being said, consider undici as well. Undici is the official Fetch API implementation that Node.js uses, and they expose the Headers class as well. This is the best option in terms of compatibility because it's literally what's being used in Node.js. I'll leave it to your judgment to decide whether you want to rely on experimental API though.

[kentcdodds]

Thanks @kettanaito! I know the team has explored undici in the past and I think there were some issues around spec compliance which led to this polyfill.

IMO, your headers-polyfill project sounds like a good one for Remix.

[kettanaito]

It would certainly be my pleasure for Remix to adopt headers-polyfill and iterate on it together to build a solid, specification-compliant polyfill on the web.

There's also a number of transformation utilities in headers-polyfill to help work with headers representations across Node.js (http.OutoingHttpHeaders, string headers in XMLHttpRequest, etc) to normalize their handling. Those have been tremendously useful during the older Node versions support.

[brophdawg11]

Awesome - yeah I would choose the MSW polyfill over undici as well. Let me play around with this a bit and see what it looks like if we switch to the MSW Headers polyfill in @remix-run/web-fetch. The goal of our polyfill was to align as close as possible to the spec - so assuming the MSW polyfill has the same goals (which I believe it does), I think it could be a good fit :)

It's also worth noting that in Remix v2 we'll no longer install our polyfill automatically, and leave it up to the user to install so they can use native fetch directly if they prefer.

[brophdawg11]

Initial pass look promising: remix-run/web-std-io#50

Only a handful of Remix E2E tests fail with this - mostly related to the ordering of serialized headers in assertions, so I'll dig into that a bit.

[kettanaito]

@brophdawg11, our intentions are certainly aligned! I'd love to hear about any issues you find out in terms of spec-compliance in headers-polyfill. For now, I've opted into Undici's headers in the upcoming release of MSW but if you discover something, please let me know. It'd help a lot. Thanks!

[brophdawg11]

Thanks @kettanaito!

It looks like the ordering is a bug - per the spec entries() should come back in a sorted manner:

The value pairs to iterate over are the return value of running sort and combine with this’s header list.

And then another small issue our tests exposed is that entries is incorrectly combining the set-cookie values, when they should be kept separate.

I'll take a quick stab at a PR to headers-polyfill :)

Here's the quick comparison program I ran to confirm (I also ran in Chrome and it matches node's built-in fetch and current @remix-run/web-fetch behavior)

const { Headers: HeadersRemix } = require("@remix-run/web-fetch");
const { Headers: HeadersPolyfill } = require("headers-polyfill");

function testHeaders(HeadersConstructor, label) {
  let h = new HeadersConstructor();
  h.set("X-B", "1");
  h.set("X-A", "2");
  h.append("X-C", "3");
  h.append("X-C", "4");
  h.append("Set-Cookie", "e=5");
  h.append("Set-Cookie", "d=6");
  console.log(`Using ${label}:\n`, JSON.stringify(Array.from(h.entries())));
}

testHeaders(Headers, "node builtin");
testHeaders(HeadersRemix, "@remix-run/web-fetch");
testHeaders(HeadersPolyfill, "headers-polyfill");

[brophdawg11]

PR: mswjs/headers-polyfill#68

With this, the entire Remix test suite passed when we used headers-polyfill directly in @remix-run/web-fetch (remix-run/web-std-io#50)

[rphlmr]

@brophdawg11

@remix-run/web-fetch is missing getSetCookie in Response (used by handleDocumentRequestRR)

I don't know if it is still mandatory to use it in entry.server.tsx but it breaks HonoJS integration.

Ref: honojs/node-server#58

[kettanaito]

@brophdawg11, that's a phenomenal work! 🎉 I will look into that pull request and let's have it released.

[kinggoesgaming]

I am assuming that this will close with singleFetch and native fetch?

[brophdawg11]

Yes this issue is obsoleted with the installGlobals({ nativeFetch: true }) flag. Anyone on Node 20+ can remove any calls to installGlobals and juts use the built-ins.