Headers polyfill is missing properties and incorrectly adds getAll #7067

kentcdodds · 2023-08-04T17:08:21Z

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.

The text was updated successfully, but these errors were encountered:

haines · 2023-08-25T14:23:52Z

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 · 2023-09-07T02:30:12Z

The types have been updated in TypeScript

kentcdodds · 2023-09-07T02:31:29Z

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 · 2023-09-07T19:22:49Z

@brophdawg11 This also breaks all (v2) TS templates

brophdawg11 · 2023-09-07T21:01:00Z

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 · 2023-09-07T21:43:55Z

Thanks for explaining that!

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

kettanaito · 2023-09-07T22:09:33Z

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 · 2023-09-07T22:56:17Z

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 · 2023-09-08T09:07:20Z

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 · 2023-09-08T14:55:12Z

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 · 2023-09-08T15:59:15Z

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 · 2023-09-08T18:20:58Z

@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 · 2023-09-08T20:02:30Z

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 · 2023-09-08T20:37:37Z

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 · 2023-09-12T13:23:55Z

@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 · 2023-09-12T18:40:08Z

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

Two compilation errors could not yet be fixed: - @remix-run/web-fetch still throws a compilation error concerning the Headers type (Known issue to remix developers: remix-run/remix#7067) - @testing-library/cypress/types throws a compilation error. Did not yet look into it but may be caused by conflicting globals of cypress and jest. Apart of the compilation errors the app throws an error on build time concerning the use of commonJS syntax (SyntaxError: Cannot use import statement outside a module). Thats a tricky one and ive not found a solution yet. Resources: - remix-run/remix#4234 - remix-run/remix#4309 - https://community.fly.io/t/cannot-use-import-statement-outside-a-module/13533 - https://stackoverflow.com/questions/58384179/syntaxerror-cannot-use-import-statement-outside-a-module (Maybe unrelated) - https://remix.run/docs/en/main/guides/gotchas (Maybe unrelated) Last but not least, the tests are not running, too. Did not yet look into it deeply but it seems it also has something to do with the globals of cypress and jest that overlap. The error Messages point to jest-setup.ts installGlobals() call.

* run npx upgrade-remix latest * Breaking change: LoaderArgs and ActionArgs are now LoaderFunctionArgs and ActionFunctionArgs * fix typescript compilation errors except thos in api workspace and in node_modules * Breaking Change: DataFunctionArgs is now LoaderFunctionArgs or ActionFunctionArgs * Exclude api from root tsconfig because it has its own tsconfig allowing decorators and stuff... * fetcher.state on <fetcher.Form method="get"> now returns "loading" instead of "submitting" (As its calling the loader not an action) * Fix typescript errors concerning remix-utlis package caused by upgrading remix - Only tsc issue inside remix-utils that is missing is the JSX type on React (React.JSX). But this is also an tsc issue inside the @remix-run/react package * fix type issue on move-to-participants * Updated all packages and fixed almost all typescript compilation errors Two compilation errors could not yet be fixed: - @remix-run/web-fetch still throws a compilation error concerning the Headers type (Known issue to remix developers: remix-run/remix#7067) - @testing-library/cypress/types throws a compilation error. Did not yet look into it but may be caused by conflicting globals of cypress and jest. Apart of the compilation errors the app throws an error on build time concerning the use of commonJS syntax (SyntaxError: Cannot use import statement outside a module). Thats a tricky one and ive not found a solution yet. Resources: - remix-run/remix#4234 - remix-run/remix#4309 - https://community.fly.io/t/cannot-use-import-statement-outside-a-module/13533 - https://stackoverflow.com/questions/58384179/syntaxerror-cannot-use-import-statement-outside-a-module (Maybe unrelated) - https://remix.run/docs/en/main/guides/gotchas (Maybe unrelated) Last but not least, the tests are not running, too. Did not yet look into it deeply but it seems it also has something to do with the globals of cypress and jest that overlap. The error Messages point to jest-setup.ts installGlobals() call. * fix: jest setup The tests are now running (34 Failed, 21 Passed). It seems all tests that failed, did that because of importing remix-utils or date-fns-tz package. * fix: tsc error inside testing-library package Had to add "cypress" to types field of tsconfig to solve this issue. See: - remix-run/indie-stack#239 - https://docs.cypress.io/guides/tooling/typescript-support#Configure-tsconfigjson - cypress-io/cypress#26930 (comment) * fix: tsc error in common/api * Update .nvmrc to version 20 * Use v1 route convention via package @remix-run/v1-route-convention * Downgrade date-fns v3 to v2 as its still incompatible with date-fns-tz v2 see: marnusw/date-fns-tz#260 * fix: build error for import statements of package remix-utils * Clean up remix.config.js * Remove cypress * fix: cjs errors in config files * Transform .eslintrc.js from cjs to esm * fix: failed to load eslint config * Change node engine in package.json to >=16 as its needed for esm support see https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c * Remove deprecated package remix-domains and use domain-functions * Removed unused packages "remix-auth" and "remix-auth-supabase" * fix: Error "imgproxy is not a constructor" fixed via serverDependencyBundling in remix.config.js * Add entry.server functionality from remix docs and fix npm start script * Write test specification * fix: zod.enum() field does not set the value on remix-forms, use z.string() instead * fix: session must be set before update password on set-password.tsx * write test specifications for reset/confirm-password.tsx * fix: set-password action could not set new session and update password in the same action used the adminAuthClient in combination with refreshSession to make this possible * update set-password test specification to fit new behaviour * Update test specification for register/verify.tsx * Replace deprecated npm package "piwik-tracker" with the new one "matomo-tracker" * breaking change: createServerClient() moved from "@supabase/remix-auth-helpers" to "@subase/ssr" In this commit i updated the createAuthClient() method. Sadly i had to change every route where the authClient is used... The auth flow has also changed and will be implemented in the next commits. As i already touched so much files i also made eslint a little happier by resolving eslint errors (mostly const instead of let) * breaking change: rework login and logout due to new createServerClient from "@supbase/ssr" * Use root response headers to refresh the session instead of every single route * Use tsx to execute scripts instead of ts-node which supports esm only * replace deprecated functions from faker inside seed script * set up local supabase for new pkce flow * Add login_redirect param to supabase templates * fix: login redirect not working with the new template structure * implement new reset password flow * add new confirm route * breaking change: use useBlocker() instead of usePrompt() to alert on discarding changes on settings * fix: drop down behind main content * feat: replace imgproxy - use own implementation - remove imgproxy-node * fix: count up throws error * fix: imgproxy returns forbidden * feat: i18n (next) (#1254) * fix: __dirname is not defined in ES module scope * feat: update tailwindcss * fix: Add missing error message on failed login (#1252) * feat: extend event data on api next (#1257) * fix: Unable to compile TypeScript Try `npm i --save-dev @types/matomo-tracker` if it exists or add a new declaration (.d.ts) file containing `declare module 'matomo-tracker'; * extend events with responsible orgs and counts * feat: add direct url to prisma (#1259) * fix: optimize queries on explore, search and detail pages (#1260) * feat: integrate sentry (next) (#1262) * feat: Improve bootstrap doc (next) (#1250) * fix: some placeholder text in readme * fix: entry.client.tsx process is not defined (#1264) * fix: Text content did not match (Hydration Error) (#1267) * feat: welcome email after registration (#1268) * feat: prevent scroll reset on several menus and tab bars (#1270) * fix: missing label for organizationType select on organization settings (#1273) * fix: typo on project settings web-social (#1275) * fix: unable to deploy api (#1277) * fix: API throws Invalid URL (#1282) * fix: wrong click radius on nav bar menu logout button (#1280) * Allow minor and patch updates for remix packages via "^<version>" inside package.json deps * Rebuild package-lock.json * fix: wrong click radius on nav bar menu logout button * fix: missing line clamp on event cards on profile detail (#1283) * fix: cannot read properties of undefined reading id on root (#1286) * fix: i18n no cache seed breaks deployment (#1288) --------- Co-authored-by: nac62116 <56918308+nac62116@users.noreply.github.com> Co-authored-by: Achim Gosse <achim.gosse@digitalnoise.de> Co-authored-by: Peter Holló <hello@songsforthe.dev>

kentcdodds added the bug:unverified label Aug 4, 2023

brophdawg11 added feat:fetch Issues related to @remix-run/web-fetch feat:typescript labels Aug 7, 2023

brophdawg11 added the v2 Issues related to v2 apis label Aug 18, 2023

brophdawg11 assigned jacob-ebey Aug 18, 2023

dusty mentioned this issue Aug 21, 2023

session dusty/remix-hono-experiment#1

Open

patrickDouglas mentioned this issue Aug 31, 2023

Typescript setup not working as expected Shopify/shopify-app-template-remix#320

Closed

brophdawg11 removed the v2 Issues related to v2 apis label Sep 5, 2023

brophdawg11 assigned brophdawg11 and unassigned jacob-ebey Sep 8, 2023

brophdawg11 added bug Something isn't working and removed bug:unverified labels Sep 8, 2023

brophdawg11 linked a pull request Sep 8, 2023 that will close this issue

POC to replace Headers with headers-polyfill remix-run/web-std-io#50

Draft

MichaelDeBoey mentioned this issue Sep 8, 2023

New Remix.run projects start with type errors #7384

Closed

1 task

alisd23 mentioned this issue Sep 24, 2023

Cannot set multiple cookies with express server #7527

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Headers polyfill is missing properties and incorrectly adds getAll #7067

Headers polyfill is missing properties and incorrectly adds getAll #7067

kentcdodds commented Aug 4, 2023 •

edited

haines commented Aug 25, 2023

kentcdodds commented Sep 7, 2023

kentcdodds commented Sep 7, 2023

MichaelDeBoey commented Sep 7, 2023 •

edited

brophdawg11 commented Sep 7, 2023 •

edited

kentcdodds commented Sep 7, 2023

kettanaito commented Sep 7, 2023 •

edited

kentcdodds commented Sep 7, 2023

kettanaito commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023

kettanaito commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023 •

edited

brophdawg11 commented Sep 8, 2023 •

edited

rphlmr commented Sep 12, 2023

kettanaito commented Sep 12, 2023

Headers polyfill is missing properties and incorrectly adds getAll #7067

Headers polyfill is missing properties and incorrectly adds getAll #7067

Comments

kentcdodds commented Aug 4, 2023 • edited

What version of Remix are you using?

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

Steps to Reproduce

Expected Behavior

Actual Behavior

haines commented Aug 25, 2023

kentcdodds commented Sep 7, 2023

kentcdodds commented Sep 7, 2023

MichaelDeBoey commented Sep 7, 2023 • edited

brophdawg11 commented Sep 7, 2023 • edited

kentcdodds commented Sep 7, 2023

kettanaito commented Sep 7, 2023 • edited

kentcdodds commented Sep 7, 2023

kettanaito commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023

kettanaito commented Sep 8, 2023

brophdawg11 commented Sep 8, 2023 • edited

brophdawg11 commented Sep 8, 2023 • edited

rphlmr commented Sep 12, 2023

kettanaito commented Sep 12, 2023

kentcdodds commented Aug 4, 2023 •

edited

MichaelDeBoey commented Sep 7, 2023 •

edited

brophdawg11 commented Sep 7, 2023 •

edited

kettanaito commented Sep 7, 2023 •

edited

brophdawg11 commented Sep 8, 2023 •

edited

brophdawg11 commented Sep 8, 2023 •

edited