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.

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

fix(routing): actions should redirect the original pathname #12173

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

ematipico
Copy link
Member

Changes

Closes #12146
Closes PLT-2576

An Astro action uses context.url.pathname for its redirect, however this is a bug because when there's a rewrite, the APIContext gets updated in place when using `next(/payload/). Astro actions need to redirect to the same original URL where the initial request started.

To fix the bug, we inject a well known header when we create RenderContext, so no matter how many rewrites happen, the original pathname is always there and the same

Testing

Created a new e2e test

Docs

N/A

Copy link

changeset-bot bot commented Oct 9, 2024

🦋 Changeset detected

Latest commit: d1fc9c2

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 9, 2024
@bluwy
Copy link
Member

bluwy commented Oct 10, 2024

Didn't notice the tests are failing 🙈 Could be related to copying the request

@ematipico
Copy link
Member Author

Didn't notice the tests are failing 🙈 Could be related to copying the request

Most probably, yeah. I think it's fine to not try to copy the body, and copy the request normally. Let's see if that works

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Test fix looks great! The remaining fail seems expected from main

packages/astro/e2e/actions-blog.test.js Outdated Show resolved Hide resolved
@ematipico ematipico merged commit 2d10de5 into main Oct 11, 2024
4 checks passed
@ematipico ematipico deleted the fix/rewrites-and-actions branch October 11, 2024 13:47
@astrobot-houston astrobot-houston mentioned this pull request Oct 11, 2024
bluwy added a commit that referenced this pull request Oct 12, 2024
bluwy added a commit that referenced this pull request Oct 12, 2024
@astrobot-houston astrobot-houston mentioned this pull request Oct 12, 2024
ematipico added a commit that referenced this pull request Oct 15, 2024
* fix(routing): actions should redirect the original pathname

* decode header to avoid index out of bounds

* fix e2e test

* Update packages/astro/e2e/actions-blog.test.js

Co-authored-by: Bjorn Lu <[email protected]>

---------

Co-authored-by: Bjorn Lu <[email protected]>
@sergeysova
Copy link

Looks like I have similar crash in production:

[info]TypeError: immutable
[info]    at appendHeader (node:internal/deps/undici/undici:8550:15)
[info]    at _Headers.append (node:internal/deps/undici/undici:8752:16)
[info]    at NodeApp.render (file:///class/dist/server/chunks/_@astrojs-ssr-adapter_DTyDJ9ui.mjs:870:26)
[info]    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
[info]    at async file:///class/dist/server/chunks/_@astrojs-ssr-adapter_DTyDJ9ui.mjs:1204:30

astro: 4.16.7

@sergeysova
Copy link

Downgrade to 4.16.6 fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite leads to 404 followed by 302 when using Server Actions in Forms
4 participants