-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(worker): prevent load vite client #12995
Conversation
Run & review this pull request in StackBlitz Codeflow. |
/ecosystem-ci run |
This comment was marked as outdated.
This comment was marked as outdated.
|
||
load(id) { | ||
if (id === importAnalysisHelperId) { | ||
return 'export ' + __vite__injectQuery.toString() |
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 don't know if this is genius or if this language has way to much meta programming 😄
For info this is fixing only some of the cases. If a plugin adds HMR code to a file imported by a worker, this will still load the client |
/ecosystem-ci run nuxt |
/ecosystem-ci run nuxt |
It seems the Nuxt issue is caused by this PR, it is working fine against main: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/4797522268/jobs/8534613140 @bluwy maybe this change is a bit big for a patch. We could merge #12988 to avoid the regression and then queue this PR for Vite 4.4 (and maybe we start it soon) |
Arnaud might be right that it's causing the failure in Nuxt. I think I'll revert that for now and preserve the extra guards. |
Co-authored-by: patak <matias.capeletto@gmail.com>
/ecosystem-ci run nuxt |
Didn't notice the fail. I'm not really sure what's going on, but I'm fine merging #12988 first then revisiting this again. |
@@ -734,7 +768,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { | |||
|
|||
if (needQueryInjectHelper) { | |||
str().prepend( | |||
`import { injectQuery as __vite__injectQuery } from "${clientPublicPath}";`, | |||
`import { __vite__injectQuery } from "${wrapppedImportAnalysisHelperId}";`, |
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.
(As mentioned in issue) Inlining the function could help solve #11266. If we don't want to inline the function for all files – we could detect classic workers via the import suffixes.
e.g.
const importSuffixes = new URL(importer, 'http://vitejs.dev').searchParams
const isClassicWorker = importSuffixes.has('worker_type') && importSuffixes.has('type', 'classic')
if (isClassicWorker) {
// Inlines the injectQuery function to the top of the worker file
str().prepend(__vite__injectQuery.toString())
} else {
// prepend import string
}
Description
fix #12970
Make sure
__vite__injectQuery
util does not bring in/@vite/client
in worker file context. This PR makes__vite__injectQuery
it's own virtual module to prevent it.I also reverted the worker guards introduce in #9064(Preserve for now to not break things as they could still be loaded by HMR code)Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).