fix: Defer warnings until after vite warning message #5563
Conversation
🦋 Changeset detectedLatest commit: f77786a 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 |
Co-authored-by: Bjorn Lu <[email protected]>
|
I don't think there would be any warnings printed in |
packages/kit/src/vite/index.js
Outdated
| @@ -76,6 +76,9 @@ export function sveltekit() { | |||
| * @return {import('vite').Plugin} | |||
| */ | |||
| function kit() { | |||
| /** @type {import('vite').UserConfig} */ | |||
| let original_config; | |||
There was a problem hiding this comment.
i'm actually surprised this works because i've run into Vite mutating the config
original_vite_config might be a clearer name to differentiate from the svelte config
There was a problem hiding this comment.
you're right, vite does mutate the config, or at least if other plugins mutate the config instead of returning a config object 🤔 I think it's rare enough that maybe it's fine for simplicity now. otherwise we'd have to deep clone it, or pre compute the warnings logs beforehand.
There was a problem hiding this comment.
vite config hook docs explicitly mention that plugins are allowed to mutate the config that is passed in
To get the unmodified one our best shot is reading the config again, the next best thing is a 'pre' plugin that clones it in it's own config hook. (hoping no other modifying plugin has run before it)
https://vitejs.dev/guide/api-plugin.html#vite-specific-hooks
There was a problem hiding this comment.
otoh even if it wasn't a user modification, the config that kit receives is the effective one, so it may still be useful to let the user know that something has a different idea. If they are using a plugin that needs a config that sveltekit does not allow the two are incompatible.
There was a problem hiding this comment.
My concern was more that we read it, then it's mutated, and we no longer know what the original config is because our version has been mutated
We can't clone the entire config because it has functions and things in it that are hard to clone, but I think the fields we need are all simple fields, so we could first filter out the fields we need and then clone those
|
As the timeout isn't completely reliable, I wonder if it'll be used for anything other than logging, so maybe we can patch print urls instead? It works without setTimeout as well that way. |
|
I think @gtm-nayan is correct. This sort of patching gives me a headache, but it seems to work consistently and it's better than the other solution. |
Co-authored-by: Simon H <[email protected]>
|
I tweaked the implementation a bit — functions that mutate external state as a side-effect make me very nervous, doubly so if it sometimes has that side-effect but sometimes has a very different side-effect (logging) depending on an optional argument. It's a bit Zalgo-y, and it's harder for things like TypeScript to make sense of. Even better, by just returning the formatted warning (for the caller to do with as they will) we end up with a bit less code. Monkey-patching |
Closes #5540.
Shamelessly stole some code from @bluwy and @dominikg. In order to defer, we need to add a listener to the server, so I moved the warning into the
configureServerfunction. Right now, it'll only work in dev, but I can probably work out a solution to add it to preview as well if necessary.It's written so that if we ever need to do other stuff after the server starts listening we can pretty easily add it.
I wasn't really sure how to test this, unfortunately. Not sure if it's worth trying?
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0