Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: allow cookies to be set in rewritten responses #11280

Merged
merged 3 commits into from
Jun 20, 2024
Merged

fix: allow cookies to be set in rewritten responses #11280

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e47e3f5
fix: allow cookies to be set in rewritten responses
ascorbic Jun 18, 2024
8f9fb35
Merge cookies
ascorbic Jun 18, 2024
a5412a7
Add support for endpoints and more tests
ascorbic Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/big-eyes-share.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug that prevented cookies from being set when using experimental rewrites
13 changes: 13 additions & 0 deletions packages/astro/src/core/cookies/cookies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,19 @@ class AstroCookies implements AstroCookiesInterface {
}
}

/**
* Merges a new AstroCookies instance into the current instance. Any new cookies
* will be added to the current instance, overwriting any existing cookies with the same name.
*/
merge(cookies: AstroCookies) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this method, I would prefer to return a new instance instead of manipulating the existing one. Essentially, try to use a pure/functional approach and avoid side-effects.

I would called this method fork instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? In this case there are already 2 AstroCookies instances created (once for each response), what does adding a 3rd accomplish? This is not immutable style code, and this function is not mutating the argument, it's mutating itself.

Copy link
Member

@ematipico ematipico Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct fix. I think the fix should be done here:

astro/packages/astro/src/core/render-context.ts

Line 237 in bc3c329

this.cookies = new AstroCookies(this.request);

And here

astro/packages/astro/src/core/render-context.ts

Line 432 in bc3c329

this.cookies = new AstroCookies(this.request);

And the code should be

this.cookies = this.cookies.fork(this.request)

fork will return a new instance of AstroCookies, and the creation of these new cookies is confined inside the rewrite function, while the proposed solution seems to be more broad

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that plan makes sense then. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think will help with cookies that are set on the response. It only merges the request cookies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to change it to a pure function, but I don't think it can be done at the point the cookies are first assigned.

Copy link
Member

@ematipico ematipico Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What confuses me is the term "rewriting a response" from the initial issue. Astro.rewrite, actually, rewrites a Request, and you'll get Response, eventually. But there's no "rewritten response" involved. So I wonder how the issue is related to rewriting? I am a bit confused 😅 maybe I am missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It refers to responses to a rewritten request. If page /a has Astro.rewrite('/b'), and /b sets a cookie, currently the cookie from b is not returned.

const outgoing = cookies.#outgoing;
if (outgoing) {
for (const [key, value] of outgoing) {
this.#ensureOutgoingMap().set(key, value);
}
}
}

/**
* Astro.cookies.header() returns an iterator for the cookies that have previously
* been set by either Astro.cookies.set() or Astro.cookies.delete().
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/cookies/response.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export function responseHasCookies(response: Response): boolean {
return Reflect.has(response, astroCookiesSymbol);
}

function getFromResponse(response: Response): AstroCookies | undefined {
export function getFromResponse(response: Response): AstroCookies | undefined {
let cookies = Reflect.get(response, astroCookiesSymbol);
if (cookies != null) {
return cookies as AstroCookies;
Expand Down
6 changes: 6 additions & 0 deletions packages/astro/src/core/render-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
responseSentSymbol,
} from './constants.js';
import { AstroCookies, attachCookiesToResponse } from './cookies/index.js';
import { getFromResponse } from './cookies/response.js';
import { AstroError, AstroErrorData } from './errors/index.js';
import { callMiddleware } from './middleware/callMiddleware.js';
import { sequence } from './middleware/index.js';
Expand Down Expand Up @@ -163,6 +164,11 @@ export class RenderContext {
streaming,
this.routeData
);

const responseCookies = getFromResponse(response);
if (result.cookies && responseCookies) {
result.cookies?.merge(responseCookies);
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
}
} catch (e) {
// If there is an error in the page's frontmatter or instantiation of the RenderTemplate fails midway,
// we signal to the rest of the internals that we can ignore the results of existing renders and avoid kicking off more of them.
Expand Down
7 changes: 7 additions & 0 deletions packages/astro/test/astro-cookies.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ describe('Astro.cookies', () => {
assert.equal(response.headers.has('set-cookie'), true);
}
});

it('can set cookies in a rewritten request', async () => {
const response = await fixture.fetch('/from');
assert.equal(response.status, 200);

assert.equal(response.headers.get('set-cookie'), 'my_cookie=value');
});
});

describe('Production', () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/test/fixtures/astro-cookies/src/pages/from.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
console.log('cookie value: ', Astro.cookies.get('my_cookie')?.value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add another test where in the first page we set a lorem value, then in the rewritten page we set a ipsum value, and assert that the cookie has ipsum as value?

return Astro.rewrite('/rewrite-target');
---
16 changes: 16 additions & 0 deletions packages/astro/test/fixtures/astro-cookies/src/pages/rewrite-target.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
Astro.cookies.set('my_cookie', 'value')
---

<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="icon" type="image/svg+xml" href="/favicon.svg" />
<meta name="viewport" content="width=device-width" />
<meta name="generator" content={Astro.generator} />
<title>Page 2</title>
</head>
<body>
<h1>Page 2</h1>
</body>
</html>
Loading