Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add entrypoint tracing #25538
Add entrypoint tracing #25538
Changes from 13 commits
5ddd237
7645c51
2c8df87
963a210
6d4bd43
5b1ed01
11d1c38
61fb021
03f1235
3b5c026
9376530
20aafbc
8b69270
7419cd7
3ab3cd9
02cfd01
21f1752
71ccfd2
3c1e604
8ae2785
b2a6b41
0dbf716
620121f
1631552
e401004
7cab94e
a2fb21f
010ee05
a0138e9
cbd1b3a
4829836
0aa38fd
98daa68
55111a3
1f1f6e3
076d001
8bae9aa
b0133b8
5cfee37
9823923
2997ee2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you should iterate over all
entry.dependencies
to handle cases likeentry: { "pages/a": ["./pages/a.js", "./something-else.js"] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the files directly from fs is probably not the best idea.
A loader could have modified the source code after filesystem. Or it could be transpiled from a language nft doesn't understand.
There is a
NormalModule.originalSource()
function which gives you the source code for a module after loader processing. That's in a language webpack understands, so either javascript, or something else you can ignore. Best checkModule.type
forjavascript/*
to only process JS code.Note that some modules might not end up at all in the output, so best use the list of chunks from
createTraceAssets
and grab all modules from there, de-duplicate them, analyse them with nft for more references, merge and cache the results per chunk and add them to the analysis data while iterating over the chunks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we can't use the list of files from
createTraceAssets
during this hook since we need it to be immediately after transpiling with babel andcompilation.entrypoints.values()
seems to be empty at this stage. I added some additional tests and gathering the files from the entry's module dependencies seems to gather everything we need currently and allows us to use the dependencies source if available as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync fs calls might have a performance influence...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems
node-file-trace
currently relies on sync fs calls, ideally we could get these from the webpack cache so that it's not actually hitting the filesystem for most of these callsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's too naive caching. Even if the module is unchanged it could be that files that are only read by nft have changed. It's not safe that way.
I would omit that for this PR and add caching in a separate PR.
We would need to track all files accessed by nft (good that it exposes the fs access methods), create a snapshot from that, store the traces and the snapshot and validate the snapshot on restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out the caching for now noting why it's disabled, will investigate it more in a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what's going on here.
Why is the module graph followed only one level? What's about dependencies of dependencies?
Is this piece of code even needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be able to trace the transpiled version of any imported files e.g.
import util from '../util'
, I added iterating over all nested dependenciesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to ignore the user's input from
unstable_excludeFiles
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was currently applying this at the end of the build where
unstable_includeFiles
is applied in case anincludeFile
collided with anexcludeFile
we could apply it here too if it helps with performanceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it will impact performance because you can stop tracing earlier so we don't look up deps of deps.
https://github.com/vercel/vercel/blob/18bec983aefbe2a77bd14eda6fca59ff7e956d8b/packages/node/src/index.ts#L204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to the ignore config here 9823923
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or actually it seems we can't pass this to the plugin since we need the built pages to gather the page configs 🤔 we might need to make this a
next.config.js
config to allow excluding while tracingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I assumed thats what it was: a
next.config.js
config so users could exclude filesLarge diffs are not rendered by default.
Large diffs are not rendered by default.