reverting createRemixHeaders to previous fix for handling multiple cookies

[Courey]

Closes: #9657

Testing Strategy:
I ran this locally and compared the output. This was also a re-introduced bug so this change is just going back to how it was a couple of months ago. It was introduced here and that is the change I am reverting.

[changeset-bot]

🦋 Changeset detected

Latest commit: bf8fd7b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/architect	Patch
create-remix	Patch
remix	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/css-bundle	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/testing	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[remix-cla-bot]

Hi @Courey,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[brophdawg11]

Do you want to take a stab at a unit test to prevent a regression like this in the future?

[Courey]

Working on it now!

[Courey]

The challenge I am having is that this test should have covered it. But I think that it's because the issue arises when using @remix-run/web-fetch and the tests are run using native fetch (I haven't yet dug deeply into that yet). So if that is the case, I am unsure of how to write a test that runs using @remix-run/web-fetch when the others are using native fetch. Is there an already existing way to do this? Am I wrong in thinking that's the issue I'm having with writing a test that captures the scenario?

[brophdawg11]

ah ok I don't think I realized this was only an issue when using web-fetch. Let me play around with a way to try to test this with both fetch implementations.

In the meantime - would you mind rebasing this and pointing it to the dev branch since it modifies source code?

[brophdawg11]

Since createRemixHeaders is internal, how about if we expose an optional DI parameter we can use to force a Headers implementation for unit testing purposes?

export function createRemixHeaders(
  requestHeaders: APIGatewayProxyEventHeaders,
  requestCookies?: string[],
  _Headers?: typeof Headers
): Headers {
  // `_Headers` should only be used for unit testing purposes so we can unit test
  // the different behaviors of the @remix-run/web-fetch `Headers` implementation
  // and the node/undici implementation.  See:
  // https://github.com/remix-run/remix/issues/9657
  let HeadersImpl = _Headers || Headers;
  let headers = new HeadersImpl();

  for (let [header, value] of Object.entries(requestHeaders)) {
    if (value) {
      headers.append(header, value);
    }
  }

  if (requestCookies) {
    // Do the semi-colon joining manually to avoid differences between Remix
    // and node/undici `Headers` implementations
    headers.append("Cookie", requestCookies.join("; "));
  }

  return headers;
}

Then to test we can add a test for the web-fetch implementation:

import { Headers as RemixHeaders } from "@remix-run/web-fetch";
...
    it("handles multiple request cookies when using @remix-run/web-fetch", () => {
      let headers = createRemixHeaders(
        {},
        ["__session=some_value", "__other=some_other_value"],
        RemixHeaders
      );
      expect(headers.get("cookie")).toEqual(
        "__session=some_value; __other=some_other_value"
      );
    });

We'll just need to install @remix-run/web-fetch as a dependency in the architect package

[Courey]

That test does pass! However, I am encountering an issue with typing:

I'm still fiddling with it, but I figured I'd let you know why it was taking a while since you gave me the test.

[brophdawg11]

oh - yeah for the test I would just @ts-expect-error that - our old implementation is known to be missing getSetCookie 😕

[brophdawg11]

Looking good! Could you add a changeset as well so we can get this merged? Just run pnpm changeset and then choose the architect package and a patch semver bump

[Courey]

I just realized that I didn't add it to the package.json in architect! Hold on merging till I get that added please! ETA: got it!

[brophdawg11]

Thanks!

[github-actions]

🤖 Hello there,

We just published version 2.10.3-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 2.10.3 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!