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): make terser optional if renderLegacyChunk false #9453

Closed
wants to merge 2 commits into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 30, 2022

Description

terser is only needed if we're rendering legacy chunks. If renderLegacyChunks: false, we don't need terser as the existing code also actually doesn't need it, per

if (!genLegacy) {
return null
}
// @ts-ignore avoid esbuild transform on legacy chunks since it produces
// legacy-unsafe code - e.g. rewriting object properties into shorthands
opts.__vite_skip_esbuild__ = true
// @ts-ignore force terser for legacy chunks. This only takes effect if
// minification isn't disabled, because that leaves out the terser plugin
// entirely.
opts.__vite_force_terser__ = true

where const genLegacy = options.renderLegacyChunks !== false.

Ref #9435 (comment)

Additional context

So far tested manually with the legacy playground only.


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 p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 30, 2022
Comment on lines +651 to +653
if (format === 'iife') {
minify = minify ? 'terser' : false
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This function is only called in two places. One for building the modern polyfilll chunk, two for legacy polyfill chunk. They pass es and iife respectively. So this changes that modern polyfills uses esbuild instead, which should be fine.

@sapphi-red
Copy link
Member

I'm not sure about this. Most users would be using plugin-legacy for legacy chunks and those users will need to install terser by themselves. Also we don't recommend users to include polyfill automatically for modern chunks.

@bluwy
Copy link
Member Author

bluwy commented Aug 2, 2022

I think plugin-legacy is also viable for generating modern-polyfills only as there's no other official/simple way to do so.

Also we don't recommend users to include polyfill automatically for modern chunks.

I think that only applies for modernPolyfills: true which I agree, it's better to specify as an array instead so we get control of what is being polyfilled, which is what I've done in the past. (Also great for build perf too)

@sapphi-red
Copy link
Member

If someone is only using renderLegacyChunk: false + modernPolyfills: ['es.promise.finally'], I think this could be done without this plugin like this: stackblitz
This injects polyfills during dev too. But with transformIndexHtml, it could be removed for dev if needed.

@bluwy
Copy link
Member Author

bluwy commented Aug 2, 2022

Interesting, I guess doing it that way is an option too, but I think it's more laborious for those who want to quickly set it up or not familiar with the tools, and harder to achieve this trick too. A plugin would simplify all the steps. 🤔

@sapphi-red
Copy link
Member

If we want to provide this feature, I think providing it with a different plugin is better.
plugin-legacy depends on @babel/standalone which is 7.16MB (packagephobia) and this is far more bigger than terser which is 3.83MB (packagephobia).
For this feature (injecting selected polyfill), babel is not needed and if we want to cut terser, we probably should cut @babel/standalone first. (We can cut regenerator-runtime and systemjs)

@babel/standalone terser
generate modern chunk
generate legacy chunk
detect required polyfill

@bluwy
Copy link
Member Author

bluwy commented Aug 25, 2022

Closing as terser is also now required for polyfills chunk minifying since #9635

@bluwy bluwy closed this Aug 25, 2022
@bluwy bluwy deleted the legacy-terser-optional branch August 25, 2022 14:36
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants