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
feat(optimizer): holdUntilCrawlEnd option #15244
feat(optimizer): holdUntilCrawlEnd option #15244
Changes from 3 commits
1fe9d9e
aa50b8d
7f881c3
ea5d2ac
5e41616
6329a85
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.
When
noDiscovery: true
, we don't need thecrawlEndFinder
. This simplifies the logic inonCrawlEnd
.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.
In case we issue the warning to add dependencies to
optimize.include
, I think we should also ask the user to add the deps discovered while the scanner was running. The scanner may end before some of these are found during the crawling of static imports in future cold starts.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.
IIUC you mean we also log the dependencies found by the scanner? (Not the static imports crawler) I'm not sure if it would be ideal to request adding them to
optimizeDeps.include
manually. I think it would be enough to request from the crawler only.In general, I think missed dependencies could mean a bug in our code (e.g.
new URL('./somewhere', import.meta.url)
which I never got around fixing 😅 ), or aoptimizeDeps.entries
misconfiguration. So we can be a bit conservative in requesting adding tooptimizeDeps.include
and try to fix the root cause instead.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.
No, if something is found by the scanner, it is fine. I mean logging the dependencies that were not found by the scanner or manually included. But some of them may have been found before the scanner finished and right now we are also using them on the first optimize run.
discoveredDepsWhileScanning
should maybe be namedmissedDepsWhileScanning
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.
Ah ok thanks for the explanation. I don't have a preference for the names, but whichever you think fits better 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.
We could ask the user to only add the dependencies that triggered the full-page reload (the ones that have custom chunks). But adding all the dependencies is better as it will avoid a new optimize run and let the browser download these dependencies as soon as the scan+optimize step is done. So better to ask for all of them to be included.
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.
Moved the build time logic here to simplify the dev logic. The comment could also be improved and during build we don't need the debounce. We want to remove this anyways, but the change helped with this PR.