feat: bundle closing

[intrnl]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
#3881

Description

• Add close method to RollupBuild which runs closeBundle hook and prevents any further use of generate and write
• Run closeBundle hook in case of build failure
• Prevent closeBundle hook from being run when build fails in watch mode
• Call close method on CLI build completion.
• Write relevant tests
• Write relevant docs

[intrnl]

I've thought about making closed a readonly property, right now you can just reassign it to falsy value and continue any further calls to generate and write, should it be kept like this as an escape hatch?

[codecov]

Codecov Report

Merging #3883 (0805d3b) into master (92a2dfa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3883   +/-   ##
=======================================
  Coverage   97.07%   97.08%           
=======================================
  Files         187      187           
  Lines        6538     6548   +10     
  Branches     1901     1904    +3     
=======================================
+ Hits         6347     6357   +10     
  Misses        101      101           
  Partials       90       90           

Impacted Files	Coverage Δ
src/utils/PluginDriver.ts	100.00% <ø> (ø)
cli/run/build.ts	89.47% <100.00%> (+0.28%)	⬆️
cli/run/watch-cli.ts	92.00% <100.00%> (+0.10%)	⬆️
src/rollup/rollup.ts	100.00% <100.00%> (ø)
src/utils/error.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92a2dfa...0805d3b. Read the comment docs.

[intrnl]

Another consideration I made is that closeBundle is an async parallel hook, this may or may not be a bad thing? When we're closing the bundle we can expect that plugins have their stuff fully closed. Comes at a surprise as users would find themselves with random errors from trying to close the bundle, though better than an unhandled promise rejection?

[intrnl]

It's more or less ready, there's still a few more tests to be added, that covers stuff like generate and write behavior after closing, CLI, and watch mode, but I'm not sure how that should be covered.

[lukastaegert]

Thanks a lot, this was quick! Give me a few days to fully review this and answer all questions.

Another consideration I made is that closeBundle is an async parallel hook, this may or may not be a bad thing

This means that the bundle.close() function should return a Promise, and it is the duty of the caller to await that promise. I think this is how it should happen. Unhandled Promise rejections from plugins doing async cleanup that cannot be awaited would be a problem. After all, one design goal for Rollup is to make it fully embeddable, which means a wrapper should be able to handle all errors.

[lukastaegert]

This looks really nice! So what we would need to release this from my side would be:

• Refine watch mode handling, see my thoughts in the comment, what do you think? Do we need to extend documentation somewhere?
• Add missing test(s), but there is not a lot missing.

[lukastaegert]

If you want, I can certainly help with tests.

[lukastaegert]

I've thought about making closed a readonly property, right now you can just reassign it to falsy value and continue any further calls to generate and write, should it be kept like this as an escape hatch?

I guess it does not matter much for now, from my side we can leave it writable. We could designate it as readonly in the TypeScript types maybe, this would tell people that they are not supposed to mess with this but they can still override it if they need.

[intrnl]

If you want, I can certainly help with tests.

If you don't mind 😅
I'm a little confused on where to put the tests

[lukastaegert]

I added a test so coverage can pass. One question as we are currently having issues with our Windows tests not running on CI: Could you check if you can enable Github Actions for your forked repository? https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/disabling-or-limiting-github-actions-for-a-repository#managing-github-actions-permissions-for-your-repository

[intrnl]

Seems to be already enabled on my forked repository.

[intrnl]

What's the best way on handling bundle close in cli/run/watch-cli.ts? Just await it as usual?

[lukastaegert]

Seems to be already enabled on my forked repository.
Turns out the issue might have been different after all.

What's the best way on handling bundle close in cli/run/watch-cli.ts? Just await it as usual?
I guess there could be errors which we can display, otherwise I do not see any use. Maybe this is something we should actually test: If a plugin throws an error in closeBundle, then the bundle.close() Promise should be rejected.

[intrnl]

I guess there could be errors which we can display, otherwise I do not see any use

So we shouldn't close it at all for watch-cli?

Maybe this is something we should actually test: If a plugin throws an error in closeBundle, then the bundle.close() Promise should be rejected.

Which test does it belong to? Hooks or sanity checks?

[lukastaegert]

So we shouldn't close it at all for watch-cli

Of course we should close it. And we should also await it in case a plugin throws an error in that hook so that we can display that error.

Which test does it belong to? Hooks or sanity checks?

Feels more like a hooks test to me this time.

[intrnl]

hmm, not exactly sure how to cover the tests for watch-cli closing the bundle

[lukastaegert]

I will have a look

[lukastaegert]

By the way, if you are in watch mode and want to write a plugin that retains resources across builds, then there is the closeWatcher hook to tell you when you can do that. So that means:

• Resources you need until all outputs are generated should by default be discarded in the closeBundle hook
• If it makes sense to retain resources for subsequent builds in watch mode, check this.meta.watchMode in that hook and do not free resources if that is set
• In that case, free the corresponding resources in the closeWatcher hook

[lukastaegert]

Added the remaining test

[lukastaegert]

Looks good from my side now. If CI runs through and coverage looks fine, this can be released from my side.

[intrnl]

🎉