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(plugin-legacy): Remove and record polyfills in plugin post (fix #2786, #2781) #3023

Merged
merged 1 commit into from
May 10, 2021

Conversation

TobiasMelen
Copy link
Contributor

@TobiasMelen TobiasMelen commented Apr 16, 2021

Makes sure all polyfill ES imports are trimmed from legacy bundle by moving recording and removal from Program:exit to post in plugin. This seems to ensure that @babel/preset-env has found and added all polyfills from all nodes in chunk before recordAndRemovePolyfillBabelPlugin processes.

Fixes #2786 and #2781.

Additional context

I have not been able to create a test case for this. It's probably an api triggering usage polyfill import in a certain AST structure that causes the bug, but figuring out which seems hard. Instead the solution is tested by users experiencing the bug.

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.

patak-dev
patak-dev previously approved these changes Apr 16, 2021
@FrankFan
Copy link

anyone please review this PR, a project of my company need this bugfix urgently.
I'm here waiting for a new release. 👏

the same issue: legacy.278fab9b.js:1 Uncaught SyntaxError: Unexpected token import with chrome 55

@TobiasMelen
Copy link
Contributor Author

@FrankFan (and others having the issue) It's very possible to workaround this issue until merged and released by using a local version of the plugin:

@Shinigami92
Copy link
Member

Another way would be to use https://www.npmjs.com/package/patch-package

@patak-dev
Copy link
Member

@TobiasMelen would you merge main to this PR? We changed the CI configs and it should pass after re-running. Thanks!

@TobiasMelen TobiasMelen force-pushed the record-remove-polyfills-post branch from 063fccb to babd45a Compare May 3, 2021 15:50
@TobiasMelen
Copy link
Contributor Author

Ok. I made a rebase mess, but think it's in correct state now.

@marialovesbeans
Copy link

Any update on this? Can we have this PR merged?

@yjwSurCode
Copy link

@FrankFan (and others having the issue) It's very possible to workaround this issue until merged and released by using a local version of the plugin:

不生效 dev环境下 import还是报错
Import still reports an error when it does not take effect in dev environment

@patak-dev
Copy link
Member

@yjwSurCode please open a new issue if there is still something that should be fixed in core

@yjwSurCode
Copy link

@yjwSurCode please open a new issue if there is still something that should be fixed in core
can you help me? asnyc Import still reports an error when it does not take effect in dev environment about vite.plugin-legacy

@yjwSurCode
Copy link

@yjwSurCode please open a new issue if there is still something that should be fixed in core

@patak-js
can you help me? asnyc Import still reports an error when it does not take effect in dev environment about vite.plugin-legacy

@patak-dev
Copy link
Member

@yjwSurCode please use GitHub Discussions or join the chat at Vite Land to ask questions.

@yjwSurCode
Copy link

@patak-js ok

@yjwSurCode
Copy link

@yjwSurCode please use GitHub Discussions or join the chat at Vite Land to ask questions.

Why not answer?

@Shinigami92
Copy link
Member

@yjwSurCode Because we don't really track closed/merged issues/prs

@vitejs vitejs locked and limited conversation to collaborators Nov 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin-legacy:Unexpected token import
7 participants