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

fix: prevent ENOENT error on temp config removal #5198

Merged
merged 4 commits into from Nov 3, 2023

Conversation

jzempel
Copy link
Contributor

@jzempel jzempel commented Oct 13, 2023

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

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

...existing load-config-file tests continue to pass

Breaking Changes?

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

List any relevant issue numbers:

Description

We're observing rollup errors (link may archive after a few months) in a monorepo that runs parallel builds. Example:

Error: ENOENT: no such file or directory, unlink '/home/circleci/project/utils/build/rollup.config-1697053868023.cjs'

These errors are becoming more and more common as the performance of the build tooling improves. This PR substitutes the promise signature of Node unlink with the async callback signature. Doing so allows us to safely "eat" the non-critical error of attempting to delete a non-existent temporary config file.

@vercel
Copy link

vercel bot commented Oct 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 3, 2023 5:45am

jzempel added a commit to zendeskgarden/react-containers that referenced this pull request Oct 13, 2023
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

While I do not know why you have these issues (is some parallel process removing files?) I have no issues swallowing errors here. But I do not understand why you need the non-promise unlink function for this, and I do not like it as it is inconsistent.
You can also "safely eat errors" by either awaiting and writing it in a try-catch with empty handler, or append a .catch(doNothing).

Though I would actually feel better if we would still show a warning in that case as that points to an issue with your build system.

@jzempel
Copy link
Contributor Author

jzempel commented Oct 16, 2023

@lukastaegert thanks for the initial review. We are using Lerna which by default parallelizes scripting tasks across monorepo packages.

I was concerned about await here due to the code comment...

// Not awaiting here saves some ms while potentially hiding a non-critical error

But if we think that is the right way to go, that's totally fine with me. I'll update with a try..catch warning.

@jzempel
Copy link
Contributor Author

jzempel commented Nov 2, 2023

@lukastaegert do you think we have something acceptable for merge here? Thank you 🙏

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #5198 (c122ed0) into master (20a64a1) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #5198      +/-   ##
==========================================
- Coverage   98.81%   98.79%   -0.03%     
==========================================
  Files         231      231              
  Lines        8863     8866       +3     
  Branches     2317     2318       +1     
==========================================
+ Hits         8758     8759       +1     
- Misses         44       46       +2     
  Partials       61       61              
Files Coverage Δ
cli/run/loadConfigFile.ts 93.84% <50.00%> (-2.93%) ⬇️

@lukastaegert lukastaegert merged commit 2af2427 into rollup:master Nov 3, 2023
25 of 27 checks passed
@jzempel jzempel deleted the jzempel/config-unlink branch November 3, 2023 14:37
Copy link

github-actions bot commented Nov 3, 2023

This PR has been released as part of rollup@4.3.0. You can test it via npm install rollup.

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.

None yet

2 participants