-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(astro)!: stabilize failOnPrerenderConflict #14826
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
feat(astro)!: stabilize failOnPrerenderConflict #14826
Conversation
🦋 Changeset detectedLatest commit: d0e5e2b 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 |
Fryuni
left a comment
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.
Lovely! Thanks Florian!
ematipico
left a comment
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.
Awesome
Co-authored-by: Sarah Rainsberger <[email protected]>
sarah11918
left a comment
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.
Just a couple of things to note!
This is a "top-level" config option, so the JSDoc needs to be added up in that section (see the docs PR, I'm suggesting after scopedStyleStrategy, but those are just arbitrarily listed by how "popular" we think the options are. It can go anywhere in that list. (In its current location, this would show up as an env configuration option.)
I know you're reusing the error message, so you haven't added/updated anything. BUT now that we have this config option, I think the error message (https://docs.astro.build/en/reference/errors/prerender-route-conflict/) could probably be updated to show more helpful links:
- the link directly to route priority (probably more helpful than either of the existing links to
getStaticPaths()orparams?) - this new config option so that you can configure whether or not to see this message, maybe? (It can be a helpful reminder if someone sets
error, then gets annoyed and wants to silence it, if the link to the config option is directly in the error message I think?) What do you think?
Co-authored-by: Sarah Rainsberger <[email protected]>
sarah11918
left a comment
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.
LGTM! I updated all the suggestions to warn instead of warnING, in case that's helpful!
packages/astro/test/fixtures/prerender-conflict-dynamic-dynamic/astro.config.mjs
Outdated
Show resolved
Hide resolved
packages/astro/test/fixtures/prerender-conflict-static-dynamic/astro.config.mjs
Outdated
Show resolved
Hide resolved
sarah11918
left a comment
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.
LGTM! 🚀
Changes
experimental.failOnPrerenderConflict#14379experimental.failOnPrerenderConflictin favor orprerenderConflictBehaviorTesting
Updated, should pass
Docs
Changeset + withastro/docs#12745