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: bump graceful-fs and don't modify global #258

Merged
merged 4 commits into from
Jan 5, 2022

Conversation

styfle
Copy link
Member

@styfle styfle commented Jan 5, 2022

The global patching fs for graceful-fs is bad practice and should not be used in libs like @vercel/nft.

This should only ever be done at the top-level application layer, in order to delay on EMFILE errors from any fs-using dependencies. You should not do this in a library, because it can cause unexpected delays in other parts of the program.

https://github.com/isaacs/node-graceful-fs/blob/master/README.md#global-patching

Instead, we can use graceful-fs directly without patching fs.

@styfle styfle changed the title fix: bump graceful-fs and don't modify global fix: bump graceful-fs and don't modify global Jan 5, 2022
@styfle styfle marked this pull request as ready for review January 5, 2022 21:55
@styfle styfle requested review from guybedford and ijjk January 5, 2022 21:55
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #258 (4d2ad3e) into main (c6731e3) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
- Coverage   80.68%   80.64%   -0.04%     
==========================================
  Files          13       13              
  Lines        1455     1452       -3     
  Branches      540      540              
==========================================
- Hits         1174     1171       -3     
  Misses        116      116              
  Partials      165      165              
Impacted Files Coverage Δ
src/cli.ts 83.01% <100.00%> (ø)
src/node-file-trace.ts 86.57% <100.00%> (-0.19%) ⬇️
src/utils/binary-locators.ts 15.38% <100.00%> (ø)
src/utils/special-cases.ts 88.37% <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 c6731e3...4d2ad3e. Read the comment docs.

@styfle styfle merged commit cf453a5 into main Jan 5, 2022
@styfle styfle deleted the fix-global-monkeypatch-graceful-fs branch January 5, 2022 22:57
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Jan 6, 2022
This bumps `@vercel/nft` to the latest version and consequently bumps `graceful-fs` to the latest version.

- Fixes #33003
- Related to vercel/nft#258
- Related to browserify/resolve#264

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
This bumps `@vercel/nft` to the latest version and consequently bumps `graceful-fs` to the latest version.

- Fixes vercel#33003
- Related to vercel/nft#258
- Related to browserify/resolve#264

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
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.

resolve v1.21 breaks bundled graceful-fs in nextjs
2 participants