-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Concurrent generate calls share a mutable plugin driver #3174
Comments
Digging into this, I'm seeing the code for generating the different output targets run concurrently, even though they share things like the plugin driver, which are mutable stateful things that are reset at the start of Are these supposed to run concurrently? If so, I don't think we can have a shared mutable plugin driver. (Specifically, the driver's file emitter's |
Ah yes, I may have overlooked things here. There is a So fixing this is a larger refactoring that may take some time. It is in fact a goal to have separate pluginDrivers for the outputs to be able to have output-specific plugins. This may just speed this up. For the original issue linking here, I guess the |
If this is an issue that'll be fixed in a more fundamental way soon, would temporarily disabling the error entirely be a better stopgap measure? Introducing an option that'll later just be a rudiment (I can't really imagine intentionally emitting a file multiple times in a single bundle) might be messy. |
(Or just pass the actual current bundle along to |
Fair enough :) So from my side, I have a talk upcoming next week which means it might take me up to two weeks to fix this, depending on how much refactoring is involved. Maybe an approach where bundles are passed along can be implemented more quickly. But in the long term, I do want per-module plugins, so there might be a chance I would rather go for the full solution myself if it is possible with reasonable effort. Disabling the error would be ok for the time being, though I really would want my warning back at some point. |
PR welcome I guess. |
I'll happily set up a PR. So, concretely, what would you prefer I do...
|
If you can pull off the latter, that would be awesome and prefered. Otherwise the first option. As for testing, it should be easily possible to check this scenario in a chunking-form test. |
And add a test to make sure such emits no longer error spuriously. Issue rollup#3174
Okay, I gave up on the bundle-passing idea since, indeed, that's harder than I thought and would involve half the refactor you alluded to to get anything working at all. So PR #3175 crudely disables the error. |
And add a test to make sure such emits no longer error spuriously. Issue rollup#3174
Thanks! I'll not be able to check it today but if it looks good, I'll release it as a patch the next days. |
And add a test to make sure such emits no longer error spuriously. Issue rollup#3174
And add a test to make sure such emits no longer error spuriously. Issue rollup#3174
And add a test to make sure such emits no longer error spuriously. Issue rollup#3174
* Disable errors for duplicate emitted file names And add a test to make sure such emits no longer error spuriously. Issue #3174 * * Make sure test is red with original code * Add test description * Describe better in the TODO comment what the ultimate intention is
(Updated the issue title to point at the underlying problem, now that the manifestation is fixed/worked around.) |
There's another problematic manifestation of this bug: if two outputs emit to different directories, only one of the output dirs will receive the files emitted by export default {
input: "index.js",
output: [
{
dir: "dist1",
format: "cjs"
},
{
dir: "dist2",
format: "esm"
}
],
plugins: [{
generateBundle() {
this.emitFile({type: "asset", fileName: "myfile", source: "abc"})
}
}]
} |
Would serializing the output generation be a good idea until this is properly fixed? I.e. don't actually run them concurrently? |
Yes, that definitely makes sense. PR welcome. Will still need 1-2 weeks to have something ready. |
As a temporary workaround for concurrency issues in the plugin driver. See issue rollup#3174
As a temporary workaround for concurrency issues in the plugin driver. See issue rollup#3174
Done in PR #3201 |
As a temporary workaround for concurrency issues in the plugin driver. See issue #3174
Fixing this will fix #3098 |
How Do We Reproduce?
Create a plugin with a
generateBundle
hook that callsthis.emitFile({type: "asset", fileName: "myfile", source: "abc"})
.Enable the plugin and run rollup on some code with multiple
output
targets. For example with this config file:Running rollup will produce this error:
Which, as per the discussion in #3150, is not the intended behavior in this case.
The text was updated successfully, but these errors were encountered: