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

feat: handle named imports of builtin modules #8338

Merged
merged 13 commits into from
Jun 5, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented May 26, 2022

Description

Fix #3736
Fix #8203
Ref #3715 (comment) (fix no1)

Handle import { join } from 'path' in deps and source code. (named imports)

In deps, wrap proxy with Object.create to workaround esbuild ES interop code.

In source code, transform the import to import pkg from 'path'; const { join } = pkg; (es interop)

Additional context

  • If dep does const { join } = require('path'), it would error immediately as join triggers the Proxy get trap on destructure. (ES import don't have this issue due to compiled esbuild code)
  • In builds, named imports in source code are not handled as it incurs perf overhead
  • In builds, named imports in deps are not handled, but can be supported with feat: non-blocking esbuild optimization at build time #8280

Thanks @swandir for initial inspiration for the Proxy trick. Though it has evolved a lot since then.


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 26, 2022
sapphi-red
sapphi-red previously approved these changes May 27, 2022
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Awesome! The code looks good to me.

packages/vite/src/node/optimizer/esbuildDepPlugin.ts Outdated Show resolved Hide resolved
@swandir
Copy link
Contributor

swandir commented May 29, 2022

Nice! It should fix build time errors observed when running with Yarn PnP too. I'll update #6493 once this one is merged.

@patak-dev
Copy link
Member

Looks like some of the latest changes in main are affecting the PR now

@bluwy
Copy link
Member Author

bluwy commented Jun 5, 2022

Tests should be good now. Thanks for making the updates while I was away!

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
4 participants