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

Convert fs operations from sync to async #236

Merged
merged 6 commits into from
Sep 22, 2021
Merged

Convert fs operations from sync to async #236

merged 6 commits into from
Sep 22, 2021

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Sep 22, 2021

This converts our sync fs usage to async to help with performance and to allow external resolve functions to be async as well. Noticed the synchronous resolving was taking a big chunk of time while tracing during next build so hopefully this allows us to hook into webpack's resolver and give a good performance boost.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #236 (1ef8c19) into main (95f0a04) will increase coverage by 0.19%.
The diff coverage is 88.72%.

❗ Current head 1ef8c19 differs from pull request most recent head e04da34. Consider uploading reports for the commit e04da34 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
+ Coverage   80.11%   80.30%   +0.19%     
==========================================
  Files          13       13              
  Lines        1438     1447       +9     
  Branches      538      539       +1     
==========================================
+ Hits         1152     1162      +10     
+ Misses        114      113       -1     
  Partials      172      172              
Impacted Files Coverage Δ
src/resolve-dependency.ts 69.81% <77.77%> (ø)
src/analyze.ts 86.49% <89.47%> (-0.08%) ⬇️
src/node-file-trace.ts 81.22% <93.33%> (+1.31%) ⬆️
src/utils/static-eval.ts 82.95% <95.65%> (ø)
src/utils/sharedlib-emit.ts 66.66% <100.00%> (ø)
src/utils/special-cases.ts 89.28% <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 95f0a04...e04da34. Read the comment docs.

@ijjk ijjk marked this pull request as ready for review September 22, 2021 19:04
src/analyze.ts Show resolved Hide resolved
src/node-file-trace.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/utils/is-error.ts Outdated Show resolved Hide resolved
@ijjk ijjk requested a review from styfle September 22, 2021 20:23
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I think you should add a test for async usage too 🙂

test/unit.test.js Outdated Show resolved Hide resolved
@ijjk ijjk requested a review from styfle September 22, 2021 21:49
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! 🎉

@styfle styfle added the automerge Automatically merge PR once checks pass label Sep 22, 2021
@kodiakhq kodiakhq bot merged commit 167dc29 into main Sep 22, 2021
@kodiakhq kodiakhq bot deleted the add/async-fs branch September 22, 2021 22:00
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Sep 23, 2021
This updates to the latest version of `node-file-trace` and leverages the new async fs handling with webpack. In a follow-up PR we will implement the async resolver to share resolving with webpack as well. 

x-ref: vercel/nft#236
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
This updates to the latest version of `node-file-trace` and leverages the new async fs handling with webpack. In a follow-up PR we will implement the async resolver to share resolving with webpack as well. 

x-ref: vercel/nft#236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants