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 caching with processed emitDependency #239

Merged
merged 5 commits into from
Oct 13, 2021
Merged

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Oct 13, 2021

Follow-up to #238 this ensures we handle caching for processed emitDependency calls correctly. This also enables testing of the shared caching against the integration suite as it was previously only being tested against the unit suite.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #239 (2eb3ad5) into main (184e41d) will increase coverage by 0.09%.
The diff coverage is 95.83%.

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

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   81.28%   81.37%   +0.09%     
==========================================
  Files          13       13              
  Lines        1480     1498      +18     
  Branches      546      552       +6     
==========================================
+ Hits         1203     1219      +16     
  Misses        110      110              
- Partials      167      169       +2     
Impacted Files Coverage Δ
src/node-file-trace.ts 87.12% <95.83%> (+0.12%) ⬆️

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 184e41d...c14b20e. Read the comment docs.

@ijjk ijjk changed the title Fix caching with circular emitDependency Fix caching with processed emitDependency Oct 13, 2021
@ijjk ijjk marked this pull request as ready for review October 13, 2021 15:06
@ijjk ijjk added the automerge Automatically merge PR once checks pass label Oct 13, 2021
src/node-file-trace.ts Outdated Show resolved Hide resolved

if (this.processed.has(path)) {
if (filesToEmit && cacheItem) {
this.emitDependencyCache.set(path, cacheItem.then(res => {
Copy link
Member

@styfle styfle Oct 13, 2021

Choose a reason for hiding this comment

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

Any reason why you're using cacheItem.then() instead of await cacheItem?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to chain the promise here instead of awaiting it since it can be circular and cause circular awaiting that never resolves.

src/node-file-trace.ts Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit ec1a486 into main Oct 13, 2021
@kodiakhq kodiakhq bot deleted the fix/missing-cache-items branch October 13, 2021 16:58
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Oct 14, 2021
This updates to the latest version of `@vercel/nft` which includes better shared caching between runs. 

x-ref: vercel/nft#239
x-ref: vercel/nft#238
x-ref: vercel/nft#237
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
This updates to the latest version of `@vercel/nft` which includes better shared caching between runs. 

x-ref: vercel/nft#239
x-ref: vercel/nft#238
x-ref: vercel/nft#237
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