-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: dependencies are analyzed multiple times #3154
Conversation
80426a5
to
1be1155
Compare
const analyzed: Set<string> = new Set<string>() | ||
|
||
if (url[0] === `"` && url[url.length - 1] === `"`) { | ||
const ownerFilename = chunk.fileName | ||
// literal import - trace direct imports and add to deps | ||
const addDeps = (filename: string) => { | ||
if (filename === ownerFilename) return | ||
if (analyzed.has(filename)) return | ||
analyzed.add(filename) |
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.
Thanks for reworking the PR and adding the tests.
About the name of the test, we may want to test other things and not only this infinite loop. Probably better to call it multiple-entrypoints or something that indicates that it is intended to stress test the mpa use case.
We should move analyzed inside the branch, as it is not used out of it:
const analyzed: Set<string> = new Set<string>() | |
if (url[0] === `"` && url[url.length - 1] === `"`) { | |
const ownerFilename = chunk.fileName | |
// literal import - trace direct imports and add to deps | |
const addDeps = (filename: string) => { | |
if (filename === ownerFilename) return | |
if (analyzed.has(filename)) return | |
analyzed.add(filename) | |
if (url[0] === `"` && url[url.length - 1] === `"`) { | |
const ownerFilename = chunk.fileName | |
// literal import - trace direct imports and add to deps | |
const analyzed: Set<string> = new Set<string>() | |
const addDeps = (filename: string) => { | |
if (filename === ownerFilename) return | |
if (analyzed.has(filename)) return | |
analyzed.add(filename) |
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.
Jep, good points. I changed the playground name as suggested and moved analyzed inside the if.
1be1155
to
299d63b
Compare
I seems like there is an issue in the AppVeyor pipeline, as it runs into a "JavaScript heap out of memory"-error when trying to build the "multiple-entrypoints" playground, even with my fix. I don't really know what to do with that. |
Don't worry about the memory issue, we are in the process of migrating to GitHub CI |
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 know that this is quite an edge case for most users, but for us it is really important, as it prevents us from building for production.
If this create further problems, we could consider a flag for turning it on and off 🤔
Description
This is a follow up of #2541 and the revert due to the issues #3084 #3101
I did a bit more research, and i got a "minimal" working example, where you can see our issue. I generated 25 files with many references. Additionally, I added all of those files as entrypoints and added
preserveEntrySignatures: 'strict'
which also requirespolyfillDynamicImport: false
. ( This is the config we use in our production environment, as we have several plugins in other projects which need to reference modules at runtime, not build time ). Anyways, in this configuration, thevite:import-analysis
plugins seems to run forever, as the number of files to analyze grows exponentially with the total number of files. (As mentioned before, we have about 1500 modules in our project and there are two files, which will run into this issue). You can test this out by runningyarn build
in the "infinite-deps" playground without my fix.With this fix, I more or less reintroduced the fix of #2541 but I limited the duplication check to every import of a module. I also added a test-case based on the issues #3084 and #3101
Additional context
I know that this is quite an edge case for most users, but for us it is really important, as it prevents us from building for production.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).