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): emit error for forbidden rewrite #12339

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

ematipico
Copy link
Member

Changes

Closes PLT-2607
Closes #12336

Even though the issue was reported for Astro v4, unfortunately, we can't fix it because the prerender field doesn't change there, and we fixed that issue in v5.

This PR forbids to rewrite a SSR route with a SSG route, when using the server output. That's because when the SSG routes are built, they aren't part of the server output, and they are emitted as static files. Static files are served by the host, and the Astro runtime can't retrieve them at runtime during the rewriting process. This results in a runtime error at the moment.

Because of that, I decided to simulate a similar behaviour and throw an error, with some information.

Testing

I added a new test case

Docs

I will probably send a PR to explain this limitation.

Copy link

changeset-bot bot commented Oct 30, 2024

🦋 Changeset detected

Latest commit: 047e6de

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 pkg: astro Related to the core `astro` package (scope) docs pr labels Oct 30, 2024
@sarah11918
Copy link
Member

Question: does this happen only with 'server' configured, or for any on-demand page that tries to rewrite with a prerendered page?

With the new 'static' mode, someone could still add an adapter and have an on-demand route that attempts a rewrite to a (default) prerendered page. Is that also a problem?

@lilnasy
Copy link
Contributor

lilnasy commented Oct 30, 2024

Couldn't it just be supported instead of being forbidden?
For example, Cloudflare provides a simple function to retrieve static assets.: https://developers.cloudflare.com/workers/static-assets/binding/. Similarly for node, it's a matter of reading from disk.

@ematipico
Copy link
Member Author

Couldn't it just be supported instead of being forbidden? For example, Cloudflare provides a simple function to retrieve static assets.: developers.cloudflare.com/workers/static-assets/binding. Similarly for node, it's a matter of reading from disk.

Not at this time, because as you said it's the adapter that helps with that (Cloudflare and Node.js), but the rewrite code is runtime agnostic, so we can't easily implement this as bugfix without providing an adapter feature for this use case.

We can definitely support this via adapters, with some architectural changes, but that's definitely a feature, and it requires adapters to have a role in this.

@ematipico
Copy link
Member Author

Question: does this happen only with 'server' configured, or for any on-demand page that tries to rewrite with a prerendered page?

With the new 'static' mode, someone could still add an adapter and have an on-demand route that attempts a rewrite to a (default) prerendered page. Is that also a problem?

@sarah11918 I am not really sure to be honest, I need to investigate

@ematipico ematipico linked an issue Oct 31, 2024 that may be closed by this pull request
1 task
@sarah11918
Copy link
Member

Just a note that I'll want to look over docs after we know whether we need to specifically say 'server' mode, or whether this is the case for any on-demand rendered route, even in the new 'static' mode!

@ematipico
Copy link
Member Author

@sarah11918 I looked at it, and this change affects only server adapters.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks @ematipico ! Had just wanted to make sure we needed the extra bit of wording (and that everything is fine in static mode) to know exactly what we needed to say! 🙌

Just fixed some tiny typos and had a thought about the error name for your consideration!

*/
export const ForbiddenRewrite = {
name: 'ForbiddenRewrite',
title: "Can't use `Astro.rewrite()` from a on-demand route to static route with 'server' output.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: "Can't use `Astro.rewrite()` from a on-demand route to static route with 'server' output.",
title: "Can't use `Astro.rewrite()` from an on-demand route to static route with 'server' output.",

typo fix here at the very least, but in fact, see suggestion for title below also.

*
*/
export const ForbiddenRewrite = {
name: 'ForbiddenRewrite',
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that there may eventually be other kinds of "forbidden" rewrites, so not sure whether you want to add more context like ForbiddenRewriteToStatic or something like that?

*/
export const ForbiddenRewrite = {
name: 'ForbiddenRewrite',
title: "Can't use `Astro.rewrite()` from a on-demand route to static route with 'server' output.",
Copy link
Member

@sarah11918 sarah11918 Nov 6, 2024

Choose a reason for hiding this comment

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

At the very least, the typo needs to be corrected to an on-demand route.

Also noting that this is a little longer than other titles are, and almost repeats the docs text exactly. So this could instead be a shorter thing like:

Forbidden rewrite to a static route

and then let the docs text explain what's happening?

Comment on lines +1273 to +1275
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which is can't be retrieved at runtime by Astro.`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which is can't be retrieved at runtime by Astro.`,
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which can't be retrieved at runtime by Astro.`,

/**
* @docs
* @description
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route, when using the `"server"` output.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route, when using the `"server"` output.
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route when using the `"server"` output.

Comment on lines +5 to +7
Now Astro emits an error when `Astro.rewrite` is used to rewrite an on-demand route with a static route, when using the `"server"` output.

This isn't possible because Astro can't retrieve the emitted static route at runtime, because it's served by the hosting platform, and not Astro itself.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Now Astro emits an error when `Astro.rewrite` is used to rewrite an on-demand route with a static route, when using the `"server"` output.
This isn't possible because Astro can't retrieve the emitted static route at runtime, because it's served by the hosting platform, and not Astro itself.
Adds an error when `Astro.rewrite()` is used to rewrite an on-demand route with a static route when using the `"server"` output.
This is a forbidden rewrite because Astro can't retrieve the emitted static route at runtime. This route is served by the hosting platform, and not Astro itself.

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

Successfully merging this pull request may close these issues.

TypeError: module.page is not a function when using rewrites
4 participants