-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(node): handle redirects dynamically in static mode #14441
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cf0d82a 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 |
CodSpeed Performance ReportMerging #14441 will not alter performanceComparing Summary
|
This looks pretty good to me! @ematipico can you double check if it's okay or it could have other side-effects? @sarah11918 can you let me know how we should handle the changeset(s) since it's a minor for the node adapter? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. I would amplify the changeset and explain to the users what this new feature does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this feature @upsuper ! It sound like this will be a great benefit to people using the Node.js adapter!
Since we often like to describe the benefits of a new feature for a minor PR, I made a suggestion below that is typically what we would say/how long it would be. You are the expert in your feature, not me, so it might need improving to make it accurate and true! 😄 But you can use it as a guideline for what we would normally write here!
.changeset/weak-eagles-give.md
Outdated
'astro': patch | ||
--- | ||
|
||
Handle configured redirects dynamically in static mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle configured redirects dynamically in static mode | |
Updates redirect handling to be consistent across `static` and `server` mode, aligning with the behavior of other adapters. | |
Previously, the Node.js adapter used default HTML files with meta refresh tags when in static mode. This often resulted in an extra flash of the page on redirect, while also not applying the proper status code for redirections. It's also likely less friendly to search engines. | |
This update ensures that configured redirects are handled in the same way by the `@astrojs/node` adapter for both prerendered pages and pages rendered on-demand. This also makes the Node.js adapter consistent with the other official adapters. | |
No change to your project is required to take advantage of this new adapter functionality. You should not experience any breaking changes. In fact, you should now notice smoother refreshes on redirects, with more accurate HTTP status codes, and may even see some SEO gains. |
Is this all correct? Is there NOTHING that can go wrong in someone's project with this change?? There is no behavior or functionality that they are losing that they might need to try to revert to with a particular change to their project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "output" instead of "mode", to avoid confusion with the adapter "mode" option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've updated the changeset. Hope it works better.
Hey @sarah11918, would you mind taking another look when you have time? |
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
Currently, node adapter uses the default HTML files with meta refresh when in static mode. This is not ideal, because it adds extra flash of page on redirections for users, and doesn't apply the proper status code for redirections. It's also likely less friendly to search engines.
This change makes configured redirect handled in the same way for node adapter across static and server mode, align with behaviors for other adapters.
Testing
A new test has been added to test the redirect behavior on static mode.
I checked that these tests pass on
server
output mode without any change in this PR, and code changes here make them also pass onstatic
output mode.Docs
While this changes the behavior of node adapter in static mode, it's still within the behavior described in the current document:
In Routing, it says:
And in Configuration Reference, it says:
So this change brings node adapter into the "supported adapters" in the first document, and fulfills the description by the second document.
Disclosure
This change uses help from LLM. It has helped me understand the codebase and explore possible solutions. The main code change was made by myself (because LLM struggled to find the right place but it did provide a lot of inspiration). The tests were mostly written by LLM, but I have also reviewed it carefully and removed and changed them based on my understanding of the matter.