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: dependencies are analyzed multiple times #3154

Merged
merged 1 commit into from Apr 27, 2021

Conversation

rpetersen27
Copy link
Contributor

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 requires polyfillDynamicImport: 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, the vite: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 running yarn 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@rpetersen27 rpetersen27 force-pushed the fix-prevent-multi-analyze branch 3 times, most recently from 80426a5 to 1be1155 Compare April 26, 2021 08:59
Comment on lines 234 to 242
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)
Copy link
Member

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:

Suggested change
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)

Copy link
Contributor Author

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.

@rpetersen27
Copy link
Contributor Author

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.

@patak-dev
Copy link
Member

Don't worry about the memory issue, we are in the process of migrating to GitHub CI

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Apr 26, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a 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 🤔

@antfu antfu merged commit 28a67ad into vitejs:main Apr 27, 2021
TobiasMelen pushed a commit to TobiasMelen/vite that referenced this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants