fix(open-next): parse cookies when converting response to cloudfront

[lucasvieirasilva]

This PR parse the set-cookie header when converting the response to the CloudFront format.

Current Behavior

set-cookie: test=1; Path=/; HttpOnly; Secure; SameSite=None, test=2; Path=/; HttpOnly; Secure; SameSite=None

Expected Behavior

set-cookie: test=1; Path=/; HttpOnly; Secure; SameSite=None set-cookie: test=2; Path=/; HttpOnly; Secure; SameSite=None

Tests

I've published it to my own npm org and tested it in my application:

https://www.npmjs.com/package/@sstlv/open-next/v/2.3.7-beta.0

Fixes

[#386]

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: d4182190ed6f8aaa4ee24fb49d9d31e323c276a6

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

This PR includes changesets to release 4 packages

| Name | Type | | ---------------- | ----- | | open-next | Patch | | app-pages-router | Patch | | app-router | Patch | | tests-unit | 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

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
open-next	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 22, 2024 1:30pm

[khuezy]

Thanks for the PR, can you also add an e2e test to cover this? eg: https://github.com/sst/open-next/blob/main/examples/app-router/middleware.ts#L39 set the set-cookie and validate that multiple cookies are set on the client.

[lucasvieirasilva]

Thanks for the PR, can you also add an e2e test to cover this? eg: https://github.com/sst/open-next/blob/main/examples/app-router/middleware.ts#L39 set the set-cookie and validate that multiple cookies are set on the client.

Do the e2e tests simulate the CloudFront request/response?

If not, this block is already covering multiple cookies: https://github.com/sst/open-next/blob/main/examples/app-router/middleware.ts#L62-L66

[khuezy]

Cool, I wonder if we could just add edge: true here: https://github.com/sst/open-next/blob/main/examples/sst/stacks/AppRouter.ts or if we want to duplicate the stack to cover both scenarios... probably the latter.

[lucasvieirasilva]

No, in the project that I'm using OpenNext locally always works, even without this fix, only when we deploy the issue start happening.

[khuezy]

correct, I'm saying we should add coverage for the deployed e2e case with Lambda@Edge configured.

[lucasvieirasilva]

got it, do you mean this website https://d1gwt3w78t4dm3.cloudfront.net/?

[khuezy]

I'm thinking we can just duplicate this https://github.com/sst/open-next/blob/main/examples/sst/stacks/AppRouter.ts and name it AppRouterEdge and add edge: true.

Then add the new stack here: https://github.com/sst/open-next/blob/main/examples/sst/sst.config.ts

Then update the e2e configuration: https://github.com/sst/open-next/blob/main/.github/workflows/e2e.yml#L154-L159

Finally add some e2e that targets the edge app, eg a copy of this: https://github.com/sst/open-next/blob/main/packages/tests-e2e/tests/appRouter/middleware.cookies.test.ts but for the appRouterEdge version of the coverage (eg, appRouterEdge/middleware.cookies.test.ts

@conico974 your opinions?

[conico974]

I'm not sure creating another e2e app to test only edge: true make that much sense The only difference between edge: true and edge: false from an open-next standpoint is in the event mapper.

The thing that i think we should do is to start incorporating unit test for this kind of things. If we properly setup those test, most things should be covered, and we can add the missing test afterwards. There is already a tests-unit package, we just need to create the tests and add them to the github actions.

And with V3 and splitting we won't have to move a whole app to the edge, we could only move a small part there for e2e.

[khuezy]

Fair point, since V3 is "around the corner", a unit test would suffice. My only worry is that a unit test doesn't truly cover the actual deployed services so things can go through the cracks.

But if we can create a unit test harness that covers/catches these kinds of things, that would be ideal.

[lucasvieirasilva]

@khuezy I've added some unit tests for the CloudFront event mapper.

[lucasvieirasilva]

@khuezy can we have this PR merged and released?

[conico974]

I can test, merge and release tomorrow