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(legacy): skip esbuild transform for systemjs #9635

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 11, 2022

Description

fix #9618

The gist of the issue is that the esbuild plugin is used to built the polyfill chunk which includes the systemjs code. This PR skips esbuild for it.

Additional context

I think there are many ways to tackle this, so happy to discuss the best option here. e.g. should we always avoid esbuild for polyfill chunks?

I think this may invalidate #9453 too.


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 plugin: legacy p5-urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) labels Aug 11, 2022
@sapphi-red
Copy link
Member

#8660 is causing this issue.
My opinion here is to set optimizeDeps.esbuildOptions.target = 'es5' and esbuild = false.

#8660 set build.target = 'es5' to prevent esbuild to inject es2015 code when Esbuild Deps Optimization at Build Time is enabled. We could have the same affect with optimizeDeps.esbuildOptions.target.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

This looks good to me as a hot patch, we can later review doing it in other cases too. Merging and running vite-ecosystem-ci, let's release vite and plugin-legacy

@patak-dev patak-dev merged commit ac16abd into main Aug 11, 2022
@patak-dev patak-dev deleted the fix-legacy-systemjs branch August 11, 2022 17:10
@patak-dev
Copy link
Member

@sapphi-red saw your message after writing mine, we can do another release later with a better method if you want to do that PR.

@sapphi-red
Copy link
Member

sapphi-red commented Aug 11, 2022

The difficult point is, when bundling a code with esbuild:

  • if target is not es5, the code will transpiled to es2015+
  • if target is es5, the code cannot include es2015+ code

Most code won't have any problem with transpiling es5 --(esbuild)--> es2015 --(babel)--> es5.
But for polyfill chunks, this seems sometime causes problems.

@sapphi-red
Copy link
Member

should we always avoid esbuild for polyfill chunks?

I'm not sure about this. To be safe, maybe we should use terser with these options?
https://github.com/zloirock/core-js/blob/bac1385ec346896d4f0478a125817c47a5bea2a4/scripts/bundle.mjs#L1-L41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p5-urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) plugin: legacy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@vite/plugin-legacy bug with buildPolyfillChunk and const type variable
3 participants