Skip to content
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: use the resolved vite root #247

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Conversation

ElMassimo
Copy link
Contributor

@ElMassimo ElMassimo commented Jan 6, 2022

Description 📖

This pull request fixes a compatibility issue between vite-plugin-svelte and backend integrations like Vite Ruby, by using the root provided by Vite in the configResolved hook.

Background 📜

vite-plugin-svelte needs to infer the root before configResolved in order to pre-scan svelte dependencies, which was last changed in #115.

Vite plugins can override options using the config hook. In particular, Vite Ruby changes the root option based on user configuration, which is typically a subfolder (instead of the project root, process.cwd()).

vite-plugin-svelte does not use the resolved value for root, which can cause Vite and vite-plugin-svelte to use different values of root to resolve paths.

The Bug 🐞

Since the root is different, the check for file existence when creating a CSS virtual module no longer matches the expected result, preventing CSS virtual modules from being served.

Proposed Fix 🔨

Using the root that Vite provides in the configResolved hook fixes this and other similar problems.

Notes ✏️

Since vite-plugin-svelte uses enforce: 'pre', it's not viable for other plugins to workaround this issue without this patch.

Using enforce: 'pre' in other plugins would make the outcome dependent on the order in which the plugins are applied in the user config, which would be error-prone and result in a bad user experience.

Although vite-plugin-svelte needs to infer a root in order to pre-scan
svelte dependencies, it should use the root returned in configResolved
to ensure compatibility with backend integrations and other plugins that
might modify the root.

Since vite-plugin-svelte uses `enforce: 'pre'`, it's not viable for
other plugins to workaround this by using `enforce: 'pre'`, since the
behavior would become order-dependent on how the user applies the
plugins, which is error-prone and would result in a bad user experience.
@bluwy
Copy link
Member

bluwy commented Jan 6, 2022

Hey 👋 Thanks for bring this up. This looks like a valid issue, since we have always worked under the assumption that root (and other usual user configs) are never changed by a plugin. Good to know that there's a case like this out there.

This brings another (minor) issue though. We use the root to look for svelte.config.js, and loading the Svelte config has to happen in the config hook since it would return additional Vite config. With this PR, the svelte.config.js would be loaded from the old root instead.

This likely isn't a big issue, but just noting here for future reference.


Re the change, this looks fine to me. But on our part we might have to re-think how we should load Svelte configs.

@dominikg
Copy link
Member

dominikg commented Jan 6, 2022

Thank you for this high quality PR. ❤️

Regarding enforce: pre, this has been required for vite-plugin-svelte to be able to resolve svelte correctly. We may be able to either split vite-plugin-svelte into multiple plugins where only the resolve part has 'pre' or check if changes in vite since then made it possible to do it differently.

But in the interim i'm going to merge this, as it doesn't change the behavior if root is unchanged and improves the behavior if it is.

Although i think plugins should not modify root from their config hook. This value is fundamental and should be stable for all plugins regardless or ordering. Users can provide a custom root from their config file or via cli. Custom frameworks or integrations could document how to set that up or provide a wrapper around defineConfig 🤔

Imagine a situation where 2 plugins want to modify root to different locations. Which one "wins"?

@dominikg dominikg merged commit e050541 into sveltejs:main Jan 6, 2022
@github-actions github-actions bot mentioned this pull request Jan 6, 2022
@buhrmi
Copy link

buhrmi commented Jan 6, 2022

Just updated to 1.0.0-next.34, works perfectly. Thanks guys

@ElMassimo ElMassimo deleted the resolved-root branch January 6, 2022 14:04
@github-actions github-actions bot mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants