-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: add preprocessor warning about common mistakes #8475
Conversation
Uses a preprocessor to check the script contents of Svelte files for "export const prerender/csr/ssr/trailingsSlash =" closes #7411
🦋 Changeset detectedLatest commit: e311585 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 |
.svelte
files is detected
you could add this via Is this safe though or are there false positives? Maybe only do it for dev mode? |
Would it make a difference in practice? What would be the advantage of providing it through The slot check is definitely safe, if anything it would give false negatives. The prerender thing could theoretically result in false positives if someone commented out the code, but why would you have something like that commented out. Making it dev only isn't much use, this warning is only spitted out at build time (which could be seen as a good thing), it has no effect on runtime. |
can preprocess return warnings or do you think it's possible to use onwarn here? leaving users no way out of this log feels bad. or is this something for an eslint plugin sveltekit? squiggles would be even more helpful. |
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 have mixed feelings about warning people for page options in .svelte
files. I'm assuming it's mostly people upgrading from older versions. We removed all our other upgrade / deprecation warnings with the release of 1.0, so it feels a bit out of place to add it and I'm guessing adding a preprocessor can have some performance implications
.changeset/nine-pets-join.md
Outdated
'@sveltejs/kit': minor | ||
--- | ||
|
||
feat: warn when usage of page options in `.svelte` files is detected |
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.
should also mention missing slot
in layout or make this changeset more generic to cover both cases
Yeah, I have the same reservations. For the page options, I think the cost of adding a preprocessor outweighs the benefit of helping a tiny minority of developers who (frankly) didn't read the docs. For slots, you get this error at dev time:
Perhaps we could capture that warning and replace it with an error like
We're already intercepting warnings here: kit/packages/kit/src/runtime/client/client.js Lines 1727 to 1741 in b6ca02a
|
I don't share the argument of that preprocessor slowing things down - since only |
Fair enough. I'm less concerned about performance than I am about adding more stuff to the codebase, especially something that feels so disconnected from all other compiler warnings/errors. And warnings were doubling up because files are preprocessed twice (once for server, once for client) — short of getting fancy, the only way to prevent that duplication is to prevent the same warning from being printed multiple times, which further reduces the likelihood of someone seeing the message at the right time during dev. (Though that was already a problem; it would only show up when the file is first transformed or immediately after it was edited). A dev runtime check would be much better, if we could get the error into the overlay, but we don't currently have a way to inspect components (we can check for So I don't love this, and think we should remove it as soon as a better option becomes available (I don't consider this sort of thing subject to semver constraints) but in the absence of one today we'll add it for the sake of closing those issues |
Uses a preprocessor to check the script contents of Svelte files for "export const prerender/csr/ssr/trailingsSlash =" or a missing
<slot />
in case of a layout filecloses #7411
closes #7773
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:
.