Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

fix: fix error when bundling multiple outputs #16

Closed
wants to merge 1 commit into from
Closed

fix: fix error when bundling multiple outputs #16

wants to merge 1 commit into from

Conversation

janriemer
Copy link

This fixes an error that occurs when multiple outputs are specified
for bundling.
Reason:

  • jest-worker was not cleaned up properly

Fixes #5

This fixes an error that occurs when multiple outputs are specified
for bundling.
Reason:
- jest-worker was not cleaned up properly

Fixes #5
@@ -15,13 +15,18 @@ function terser(userOptions = {}) {
})
);

let numOfBundles = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use plugin context instead of closure?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out plugin context. I didn't know it exists.
However, trying it out (basically attaching numOfBundles to this), makes the test hang, because an async operation is not cleaned up properly (worker is not cleaned up properly).
Could you please point me in the right direction or implement it yourself?
The only docs I've found on the context api is this: https://rollupjs.org/guide/en#context
However, it does not mention use of custom data.

I am also just curious why using context api is better than closure?
I am thankful for any clarification. 😊

Suggested change
let numOfBundles = 0;
return {
name: "terser",
renderStart() {
if (!this.worker) {
this.worker = new Worker(require.resolve("./transform.js"), {
numWorkers: userOptions.numWorkers
});
this.numOfBundles = 0;
}
this.numOfBundles++;
},
renderChunk(code) {
return this.worker.transform(code, minifierOptions).catch(error => {
const { message, line, col: column } = error;
console.error(
codeFrameColumns(code, { start: { line, column } }, { message })
);
throw error;
});
},
generateBundle() {
this.numOfBundles--;
// we only want to end worker on the last bundle
if (this.numOfBundles == 0) {
this.worker.end();
}
},
renderError() {
this.numOfBundles--;
// we only want to end worker on the last bundle
if (this.numOfBundles == 0) {
this.worker.end();
}
}
};

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context exists per build run. Closure exists for all usages. For example if plugin instance is used for a few builds we may get unexpected results.

const t = terser()
[
  {
    plugins: [t]
  },
  {
    plugins: [t]
  }
]

Also worker instance is already used in this way.

@janriemer
Copy link
Author

@TrySound Please tell me what I should do, so that this can be merged (see also comment above).

evs-chris added a commit to evs-chris/rollup-plugin-terser that referenced this pull request Dec 14, 2018
TrySound pushed a commit to evs-chris/rollup-plugin-terser that referenced this pull request Jan 1, 2019
TrySound pushed a commit that referenced this pull request Jan 1, 2019
This fixes an error that occurs when multiple outputs are specified
for bundling.
Reason:
- jest-worker was not cleaned up properly

Fixes #5
@TrySound
Copy link
Owner

TrySound commented Jan 1, 2019

Close in favour of #19

@TrySound TrySound closed this Jan 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaks with multiple outputs
2 participants