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(plugin-legacy): add modernTargets option #15506

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

undermoonn
Copy link
Contributor

@undermoonn undermoonn commented Jan 4, 2024

Description

When renderLegacyChunks option enabled, legacy plugin will force set build.target to ['es2020', 'edge79', 'firefox67', 'chrome64', 'safari12'] rendering modern chunks.

Additional context

fix #14527


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Jan 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy bluwy added plugin: legacy p2-nice-to-have Not breaking anything but nice to have (priority) labels Jan 8, 2024
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.

The implementation looks great! Thanks for updating it. Would you be able to update the README about the new modernTargets option too?

packages/plugin-legacy/package.json Outdated Show resolved Hide resolved
@bluwy bluwy changed the title feat(plugin-legacy): modernTargets options feat(plugin-legacy): add modernTargets option Jan 10, 2024
bluwy
bluwy previously approved these changes Jan 10, 2024
patak-dev
patak-dev previously approved these changes Jan 12, 2024
@patak-dev
Copy link
Member

@undermoonn, we discussed this PR in the last team meeting and decided to move forward with it. The only thing is that we should add a warning using config.logger.warn if modernTargets is used and renderLegacyChunks is true to avoid users having some versions of browsers between legacy and modern not being supported.

@undermoonn undermoonn dismissed stale reviews from patak-dev and bluwy via c0f3f81 January 12, 2024 13:37
@undermoonn
Copy link
Contributor Author

Thanks for replies and specific guidance! Love vite ❤️

@patak-dev patak-dev merged commit cf56507 into vitejs:main Jan 17, 2024
10 checks passed
@jderusse
Copy link

Thanks for this feature. Do you have an ETA for the next release (I'm not demanding, just asking)
@vitejs/plugin-legacy 5.2.0 is 2 months old but vite 5.0.12 has been release few days ago

@bluwy
Copy link
Member

bluwy commented Jan 25, 2024

No problem @jderusse. Thanks for the nudge, I just cut a release for plugin-legacy v5.3.0

@C-Higgins
Copy link

C-Higgins commented Jan 25, 2024

This PR does not work as intended. modernTargets is never set if renderLegacyChunks: false is used. The culprit is from this excerpt:

      if (!genLegacy || config.build.ssr) {
        return;
      }
      targets = options.targets || browserslistLoadConfig({ path: config.root }) || "last 2 versions and not dead, > 0.3%, Firefox ESR";
      isDebug && console.log(`[@vitejs/plugin-legacy] targets:`, targets);
      modernTargets = options.modernTargets || modernTargetsBabel;

Notice that the function returns before the modernTargets can be set.

This means that later on, the variable is undefined, and we're back to the original behavior:

        if (options.modernPolyfills && !Array.isArray(options.modernPolyfills) && genModern) {
          await detectPolyfills(raw, modernTargets, modernPolyfills);  // modernTargets is undefined
        }

@bluwy
Copy link
Member

bluwy commented Jan 25, 2024

Thanks for flagging that @C-Higgins. That condition does seem oddly placed, the targets and modernTargets option should probably be placed above it 🤔 Would you like to create an issue or PR for this?

@undermoonn
Copy link
Contributor Author

Sorry to that @C-Higgins I didn't check the code when renderLegacyChunks: false

Maybe I can fix it with code below

if (options.modernPolyfills && !Array.isArray(options.modernPolyfills) && genModern) {
  await detectPolyfills(raw, options.modernTargets || modernTargetsBabel, modernPolyfills);
}

Place target above that condition will load browserslist which is unnecessary in ssr mode.

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) plugin: legacy
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Legacy plugin modern polyfills don't respect targets
5 participants