vercel / nft

Node.js dependency tracing utility
https://npmjs.com/@vercel/nft
MIT License
1.31k stars 136 forks source link

major: assets should not be parsed #304

Closed styfle closed 2 years ago

styfle commented 2 years ago

This PR changes the behavior of dependency tracing such that fs.readFile('./file.js') is no longer considered a dependency and it will not be traced. Instead, it is considered an asset the same way a fs.readFile('./file.txt') is. The dependency type will be reserved for import and require only.

Note that this could be considered a breaking change since https://github.com/vercel/nft/pull/40 expected the readFile() config to be called for assets but it should only be called for dependencies.

In order to reduce the amount of breakage, require.resolve() was left as dependency instead of asset since its often passed to child_process.spawn() which needs the dependencies.

codecov[bot] commented 2 years ago

Codecov Report

Merging #304 (3c8ab23) into main (bbb2bb8) will decrease coverage by 0.29%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
- Coverage   80.57%   80.27%   -0.30%     
==========================================
  Files          13       13              
  Lines        1498     1501       +3     
  Branches      556      556              
==========================================
- Hits         1207     1205       -2     
  Misses        121      121              
- Partials      170      175       +5     
Impacted Files Coverage Δ
src/analyze.ts 86.11% <100.00%> (+0.09%) :arrow_up:
src/node-file-trace.ts 87.55% <100.00%> (-0.45%) :arrow_down:
src/utils/special-cases.ts 85.10% <100.00%> (-2.13%) :arrow_down:
src/utils/static-eval.ts 82.53% <0.00%> (-0.64%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

FBosler commented 2 years ago

I think this might be related to this and this stackoverflow post

styfle commented 2 years ago

@FBosler You shouldn't see this change unless you upgrade Next.js to 12.2.6-canary.5 or newer.

If you're using a version between 12.0.0 and less than 12.2.6-canary.4, then you want see this change.

FBosler commented 2 years ago

Yea, we upgraded to the most recent next.js version. But downgrading the Vercel CLI fixed the bug!

styfle commented 2 years ago

Reverting in #309