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: handle optimize failure #8006

Merged
merged 2 commits into from
May 4, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented May 3, 2022

Description

Stop optimize reload if fail on esbuild bundling.

Additional context

Looking into #3715 today. With Vite 2.9, when optimization fails (because of named importing a nodejs module, will also look into this soon), vite would reload optimization again, but since it would always fail, it will reload infinitely.

Question: Should we not do a fullReload all together?

Note: I also made a repro https://github.com/bluwy/vite-bundle-node-issue


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.

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 3, 2022
@bluwy bluwy requested a review from patak-dev May 3, 2022 18:51
@patak-dev
Copy link
Member

You are on fire today 🔥

I didn't modify this in the last batch of deps optimization PRs. This was always full reloading on error before: https://github.com/vitejs/vite/blame/b214115bffd07c5baca9da36d3b3608007d00920/packages/vite/src/node/optimizer/registerMissing.ts#L86. But I added a reset that is triggering the re-discovery of deps:

      // Reset missing deps, let the server rediscover the dependencies
      metadata.discovered = {}

I don't know if this reset is helping actually. But given that the user may edit files and fix a potential issue by deleting a dep import, it may avoid a full reload.

I vote to avoid full-reload altogether when there is an error, and maybe we could add a better warning stating that this is an internal error and the user may need to restart the server? I don't think we can recover from this situation.

@bluwy
Copy link
Member Author

bluwy commented May 4, 2022

It was a public holiday yesterday 😄

Yeah it looks like the reset plays a factor too. I think the reset helps that if I comment/uncomment the problematic import, the dev server is able to recover and work properly again, using the repro.

I think removing the fullReload make sense. If it fails once, it's likely to fail the same way twice. I don't think it's necessarily an internal error though, there could be libraries that are incorrectly bundled that messes up esbuild, so maybe we can leave the error as is.

@patak-dev patak-dev merged commit ba95a2a into vitejs:main May 4, 2022
@bluwy bluwy deleted the fix-infinite-bundle-reload branch May 5, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants