-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: enhance and tighten up Vercel adapter warnings #9436
Conversation
- warn when prerender setting makes isr config useless - don't show cron warning when everything's valid - allow to set isr to false to clear isr config in leafs (else it's impossible to do so because config is merged at the top level) - prefixed all warnings with "Warning:" which should help detect the Vercel log dashboard show these in condensed mode where only warnings/errors are shown
🦋 Changeset detectedLatest commit: afb36db The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@@ -87,7 +87,7 @@ export interface Builder { | |||
config: ValidatedConfig; | |||
/** Information about prerendered pages and assets, if any. */ | |||
prerendered: Prerendered; | |||
/** An array of dynamic (not prerendered) routes */ | |||
/** An array of all routes (including prerendered) */ |
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.
drive-by comment fix - I don't think we need a changeset for this, it's okay to just release it as part of the next Kit release without special mention
@@ -219,6 +226,20 @@ const plugin = function (defaults = {}) { | |||
group.routes.push(route); | |||
} | |||
|
|||
if (ignored_isr.size) { | |||
builder.log.warn( | |||
`\nWarning: The following routes have an ISR config which is ignored because the route is prerendered:` |
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 made this a warning, thought about whether or not this is an error, but I think that would be breaking - maybe in the next major? Or would this be more annoying than helpful if it was an error?
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 think a warning is probably fine, you could have a prerendered leaf within an ISR'd group, for example. we can see if people trip up on it in practice
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.
made one small suggestion but otherwise LGTM
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
prerender = 'auto'
routes are now also excempt from further processing if we can determine that this is equal toprerender = true
(because there are no dynamic routes)Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.