-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: make illegal server-only import errors actually useful #14155
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
Conversation
🦋 Changeset detectedLatest commit: 21650f4 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 |
I actually think we can get the import chain in development too, by inspecting the dev server's module graph. Tinkering on that locally |
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. Might be good to also have a test for a transitive server import as I noticed that most of the tests are just a direct import.
normalized === '$env/static/private' || | ||
normalized === '$env/dynamic/private' || | ||
normalized === '$app/server' || | ||
normalized.startsWith('$lib/server') || |
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 wonder if we can easily change this to only match $lib/server/
directories or $lib/server.*
files to fix #14069 but I guess that can be its own PR after this one
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.
ah yeah this should really just add a trailing slash. We don't state anywhere that $lib/server.ts
should be considered a server-only module — just this:
You can make your own modules server-only in two ways:
- adding
.server
to the filename, e.g.secrets.server.js
- placing them in
$lib/server
, e.g.$lib/server/secrets.js
Unfortunately we do currently regard $lib/server.ts
as server-only, so the simple fix would be a breaking change. I think we should fix that in SvelteKit 3 and warn on it in the meantime. Will do it as a separate PR
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.
Actually I've changed my mind. If we think it's okay to fix #14069 in a patch release then the same is true of $lib/server.ts
. Just gonna update .startsWith('$lib/server')
to .startsWith('$lib/server/')
fixes #14153. The one part I'm slightly nervous about is manipulating the errors that occur during the secondary build to avoid double-logging, though it's probably okay?
This is a radical simplification from what we had before. It results in consistent errors between dev and prod (no more 'build the app to figure out the import chain'), more readable errors (better formatting, more concise message, no useless stack trace), co-locates code better, is more efficient (instead of walking the entire module graph from the universal entry points, it walks up from a server-only module if/when one is found), and is a lot less code.
One very minor trade-off: it's no longer possible to differentiate between static and dynamic imports. I don't think this matters.
Not require ready to merge yet because FUCKING WINDOWSPlease 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:
.Edits