Skip to content
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

Track unfinished hook actions as regular errors #4434

Merged

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 9, 2022

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:
Resolves #4432

Description

By using the beforeExit event, this PR solves a key issue with the previous implementation to track unfinished hook actions: They are now reported as regular errors. That means that

  • There are no unexpected prints to the console if used from a script
  • Regular error formatting logic kicks in, especially when used from custom scripts
  • There is no longer any global state, but unfinished actions are tracked per build
  • There is no unexpected output when the build is manually aborted via process.exit()

@github-actions
Copy link

@github-actions github-actions bot commented Mar 9, 2022

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#gh-4432-improve-unfinished-hook-action-handling

or load it into the REPL:
https://rollupjs.org/repl/?pr=4434

@codecov
Copy link

@codecov codecov bot commented Mar 9, 2022

Codecov Report

Merging #4434 (c2638fe) into master (511e9ae) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4434   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         205      205           
  Lines        7324     7326    +2     
  Branches     2080     2079    -1     
=======================================
+ Hits         7234     7236    +2     
  Misses         33       33           
  Partials       57       57           
Impacted Files Coverage Δ
src/rollup/rollup.ts 100.00% <100.00%> (ø)
src/utils/PluginDriver.ts 100.00% <100.00%> (ø)
src/utils/hookActions.ts 94.11% <100.00%> (-0.62%) ⬇️

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 511e9ae...c2638fe. Read the comment docs.

@lukastaegert lukastaegert force-pushed the gh-4432-improve-unfinished-hook-action-handling branch from 0575d0c to e1d632a Compare Mar 9, 2022
@lukastaegert lukastaegert force-pushed the gh-4432-improve-unfinished-hook-action-handling branch from e1d632a to c2638fe Compare Mar 9, 2022
Copy link

@timvahlbrock timvahlbrock left a comment

Don't know if my review is of any value, but this looks good to me.

@lukastaegert lukastaegert merged commit 35cbfae into master Mar 14, 2022
12 checks passed
@lukastaegert lukastaegert deleted the gh-4432-improve-unfinished-hook-action-handling branch Mar 14, 2022
@Conduitry
Copy link
Contributor

@Conduitry Conduitry commented Mar 14, 2022

I think this change is resulting in me getting warnings about a lot of beforeExit handlers being added to process. My situation is probably somewhat unusual in that I'm programmatically calling the Rollup API many times in parallel. Is the best way around this probably just for me to increase process.setMaxListeners in my own code, or is there some other way to implement this that doesn't attach as many event handlers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants