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(optimizer): external require-import conversion (fixes #2492, #3409, #5308) #8459

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jun 3, 2022

Description

This PR uses esbuild's plugin hook to convert require into import even if it is externalized. (refs: #2492 (comment))

a minimal require-import conversion plugin: https://stackblitz.com/edit/node-xh6pvc?file=build.js

fixes #2492
fixes #3409
fixes #5308

I tested with #3409's repro.

Additional context

I think this approach is faster and less-fragile than regex or parsing + replacing. But it heavily depends on esbuild's plugin behavior and it might unveil an edge case bug of esbuild.


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.

@sapphi-red sapphi-red force-pushed the fix/optimizer-external-resolve-import-conversion branch from fe00b92 to 5f9d8b6 Compare June 3, 2022 16:16
@sapphi-red sapphi-red changed the title fix(optimizer): external resolve-import conversion fix(optimizer): external resolve-import conversion (fixes #2492, #3409) Jun 3, 2022
@sapphi-red sapphi-red force-pushed the fix/optimizer-external-resolve-import-conversion branch from 5f9d8b6 to 406ccfb Compare June 3, 2022 16:19
@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 3, 2022
@sapphi-red sapphi-red marked this pull request as ready for review June 3, 2022 16:28
@sapphi-red sapphi-red changed the title fix(optimizer): external resolve-import conversion (fixes #2492, #3409) fix(optimizer): external require-import conversion (fixes #2492, #3409) Jun 3, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat trick. One nitpick below but this seems like a great solution. I don't think there are cases where we would actually want to preserve the require.

packages/vite/src/node/optimizer/esbuildDepPlugin.ts Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

Nice, I agree this looks like a good move forward. Lets try it in the next alpha so we check how it works in the wild.

Tests are failing for node 16, I'll merge it anyways like the prev PRs. I hope node fix the seg fault soon or we find

@patak-dev patak-dev merged commit 1061bbd into vitejs:main Jun 5, 2022
@sapphi-red sapphi-red changed the title fix(optimizer): external require-import conversion (fixes #2492, #3409) fix(optimizer): external require-import conversion (fixes #2492, #3409, #5308) Jun 5, 2022
@sapphi-red sapphi-red deleted the fix/optimizer-external-resolve-import-conversion branch June 8, 2022 14:55
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
3 participants