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: avoid externalizing plugins that resolve to local files in monorepos #17323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frandiox
Copy link
Contributor

Context

Vite currently bundles the config file during development using ESBuild and all the relative imports are marked as "no external" because they are not matched in this regex, meaning that the imported code is read and bundled in the config.

This is great because it allows reloading the imported local plugins without restarting the process by calling viteDevServer.restart(). Otherwise, Node would cache the external imports (import cache) and wouldn't reload the code even if it changes.

Problem

We have a few plugins in a monorepo together with a project template and we'd like to reload the plugins in the template dev server when they change. The problem is that the template is importing the local plugins using NPM workspaces (e.g. published NPM name @shopify/hydrogen/vite) instead of relative paths like ../../packages/hydrogen/dist/plugin.js.

Therefore, our plugins are treated as external imports and Node's import cache makes it impossible to reload the plugin without restarting the whole dev server process.

Changes

Vite already follow symlinks in monorepos for in-app dependencies so I think it would be more consistent if it did the same for plugins in the config file. This PR makes it so imports that resolve to local files are not marked as external anymore.

I'm not sure if this is the correct fix so please let me know what you think.

Copy link

stackblitz bot commented May 27, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

It is breaking one of our test because we use a link:../package. We probably can break this test case in Vite 6, we need to solve this. I don't know if the fix is correct though. If the plugin package is optimized for example, we should continue treating it as external, no?

@frandiox
Copy link
Contributor Author

frandiox commented Jun 5, 2024

If the plugin package is optimized for example, we should continue treating it as external, no?

If it's optimized, wouldn't the path come from node_modules/.vite/xyz? Or is there any other way to check if it's optimized?

@patak-dev
Copy link
Member

The cache dir is at node_modules/.vite by default but it can be changed using config.cacheDir. An optimized dep would start with {cacheDir}/deps (without the / because we also have {cacheDir}/deps_ssr).

@bluwy
Copy link
Member

bluwy commented Jun 6, 2024

Isn't this about config bundling, so the optimizer shouldn't be in play here? The test fail is not too terrible I think and I'm ok with fixing the test.

This will fix #5370, which (seems like) I also tried this solution in the past but had issues with __filename and __dirname after bundling the deps: #5370 (comment). Maybe it's not an issue now.

@patak-dev
Copy link
Member

Isn't this about config bundling, so the optimizer shouldn't be in play here? The test fail is not too terrible I think and I'm ok with fixing the test.

Oh, you're right. Ok, ya, we can update the test then. Do you think it is safe to include this in 5.3?

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 6, 2024
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 65c037a: Open

suite result latest scheduled
histoire success failure
nuxt failure failure
qwik failure success
remix failure success
sveltekit failure failure
vike failure success
vite-plugin-react failure success
vite-plugin-react-pages failure failure
vite-plugin-vue failure success
vitest failure failure

analogjs, astro, ladle, laravel, marko, previewjs, quasar, rakkas, unocss, vite-plugin-pwa, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress

@garydex
Copy link

garydex commented Jun 6, 2024

@patak-dev @bluwy Please also check #17286 as this is a similar idea to fix the same issue (#5370), but specifically uses tsconfig paths.

Might be a more robust solution as it requires the tsconfig to be setup correctly for it to work. Might also be a sledgehammer to crack a nut :) Either way would be good to get eyes on it.

@bluwy
Copy link
Member

bluwy commented Jun 6, 2024

@garydex I've replied in the other PR


About the ecosystem-ci results, seems like it does break a few downstream frameworks, and perhaps we should move this to the next major.

@frandiox
Copy link
Contributor Author

frandiox commented Jun 10, 2024

Hm yeah, the dirname/filename issue is indeed unfortunate.
It might be a bit of an overkill but here's another idea:

  1. Get the import statements from the config file (e.g. with es-module-lexer or simple regexps for static imports).
  2. Filter out local paths (./, ../).
  3. Try to resolve each remaining import with import.meta.resolve or require.resolve.
  4. Filter out paths from /node_modules/, and take only the ones that resolve to local files
  5. Bundle the config file normally like we do right now
  6. Replace imports that resolve to local files (from step 4) in the resulting bundled config with the resolved imports but adding random query strings ?t=123 to bust the import cache.
  7. Write to disk and import it dynamically like it's done now.

Would that work? We would end up with ./something, @remix-run/dev unmodified but replace @shopify/hydrogen/vite with ../../packages/hydrogen/dist/vite/plugin.js?t=123

--

Edit: problems I see with this:

  • It might not work with package.json#exports in some situations?
  • It wouldn't work if what you're importing is not already valid JS

@ferferga
Copy link

@bluwy @patak-dev Will this be targeted to v6? Would be great to have the untranspiled monorepos config working.

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Sep 20, 2024

Though the log is not available now, when I checked it last time, vite-plugin-react ecosystem ci failure is due to "non static" dynamic import (similar to #13730). Playground's vite.config.ts is bundled with @vitejs/plugin-react as it's a workspace package, but statically-non-analyzable import("react-refresh/babel") used by @vitejs/plugin-react is not rewritten, so they become not found in playground.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants