-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: handle addWatchFile in load hooks #14967
Conversation
does not work
This reverts commit 81ba7b8.
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 think we can merge this one now, but our approach in general to communicate from one hook context to another suffers from possible race conditions. This one is similar to what we do with module.meta
. I think it would be good to have something like a pipeline context with this information passed to resolveId/load/transform to pass this information in transformRequest
at one point.
Yeah the approach here isn't the best either. A higher-level pipeline does sound like the better solution, which I think it roughly resides in It'll be a bit tricky to pass it around, or otherwise I think we can also stick with this implementation for now and improve later. |
/ecosystem-ci run |
📝 Ran ecosystem CI on
|
Lets go with this implementation for now, and we can later improve it 👍🏼 |
Description
fix #3474
The
_addedImports
of the plugin context only exists for each hook in isolation, e.g.load()
andtransform()
. InimportAnalysis.ts
, we handle_addedImports
in thetransform()
hook, so onlyaddWatchFile
intransform()
hook works.load()
was always being ignored.This PR saves the
_addedImports
fromload()
(cached viaModuleNode
weak map), and re-instates in thetransform()
hook later.NOTE: this is not supported for CSS, looks like we hardcode dependency handling for our CSS plugin only.
Additional context
https://rollupjs.org/plugin-development/#this-addwatchfile
I also tried supporting this in
resolveId()
as the Rollup docs mention that it supports it, however I reverted it (see 3rd commit) as it doesn't seem to work in Rollup (vite build --watch
). Plus the implementation to support inresolveId()
has a few downsides:resolveId
doesn't guarantee aModuleNode
yet, we have to cache by theid
string (unable to GC)resolveId
can be called but we don't necessarily want to load it. Saving the_addedImports
could pollute subsequentload
+transform
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).