Legacy plugin modern polyfills don't respect targets

[C-Higgins]

Describe the bug

It seems that with modernPolyfills: true, neither the targets nor the browserlist config is used at all. Along with polyfilling more than necessary for a given target, I am unable to get it to change the polyfill at all regardless of any targets, however new or old.

The docs say, "Note it is not recommended to use the true value (which uses auto-detection) because core-js@3 is very aggressive in polyfill inclusions due to all the bleeding edge features it supports." I understand that core-js may be aggressive, but I find it hard to believe that it thinks Chrome latest requires a polyfill for every possible feature used. In the reproduction link, notice the target is last 1 chrome version. Running the build lists a polyfill for Object.fromEntries, and includes it in the generated polyfill file, as well as something as old as Promise. The core-js compat file lists this as compatible in Chrome.

Reproduction

https://stackblitz.com/edit/vitejs-vite-sdqghd?file=counter.js

Steps to reproduce

DEBUG="vite:legacy" npm run build in linked stackblitz

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
    pnpm: 8.6.12 - /usr/local/bin/pnpm
  npmPackages:
    @vitejs/plugin-legacy: ^4.1.1 => 4.1.1 
    vite: ^4.4.8 => 4.4.9

Used Package Manager

npm

Logs

~/projects/vitejs-vite-sdqghd 2s
❯ DEBUG="vite:legacy" npm run build
rendering chunks (1)...[@vitejs/plugin-legacy] modern polyfills: Set(4) {
  'core-js/modules/es.array.iterator.js',
  'core-js/modules/web.dom-collections.iterator.js',
  'core-js/modules/es.promise.js',
  'core-js/modules/es.object.from-entries.js'
}
dist/index.html                       0.53 kB │ gzip:  0.31 kB
dist/assets/javascript-8dac5379.svg   1.00 kB │ gzip:  0.60 kB
dist/assets/index-d9d327fd.css        1.25 kB │ gzip:  0.66 kB
dist/assets/index-2ac35ad6.js         1.48 kB │ gzip:  0.78 kB
dist/assets/polyfills-e21d0655.js    26.24 kB │ gzip: 11.20 kB
✓ built in 1.30s

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[C-Higgins]

Indeed, when I modify this line to instead pass the targets instead of this hard-coded target, things work as expected

vite/packages/plugin-legacy/src/index.ts

Line 387 in 21ae371

await detectPolyfills(raw, modernTargetsBabel, modernPolyfills)

await detectPolyfills(raw, options.targets, modernPolyfills);

[blake-newman]

@C-Higgins I've litterally done the same by patching this logic with yarn patch.

Can concur that if targets is passed along side modernPolyfills: true then it should use the targets. This massively reduces the scope of polyfills that it generates.

[bluwy]

targets is the legacy target, so I don't think await detectPolyfills(raw, options.targets, modernPolyfills); is correct? Maybe we can introduce a new modernTargets option. Until there's a way for us to safely convert esbuild targets to browserslist.

[C-Higgins]

targets is the legacy target, so I don't think await detectPolyfills(raw, options.targets, modernPolyfills); is correct? Maybe we can introduce a new modernTargets option. Until there's a way for us to safely convert esbuild targets to browserslist.

I think using the passed targets must be more correct than current behavior, no? Especially if renderLegacyChunks is false. It's hard to imagine a scenario where the current behavior of modernPolyfills: true is intended and useful; it seems more like a bug to me.

[jroru]

@C-Higgins are you aware if this was this introduced in a specific release?

[jroru]

Until there's a way for us to safely convert esbuild targets to browserslist.

@bluwy this package may be useful: https://github.com/nihalgonsalves/esbuild-plugin-browserslist

[bluwy]

@C-Higgins Making targets implicitly elevate as modern targets is a bit too automatic, especially that the default targets are set to target older browsers 'last 2 versions and not dead, > 0.3%, Firefox ESR'. It would also be confusing for projects that dual-setup legacy and modern polyfills. Making a new modernTargets option, reflecting the polyfills -> modernPolyfills pattern feels more predictable to me.

@jroru We need the other way round, esbuild target -> browserslist. The esbuild plugin does the opposite.

[jgosmann]

Affected by this as well.

Also, cross-linking this comment from #6922 talking about the same problem.

[jgosmann]

We need the other way round, esbuild target -> browserslist. The esbuild plugin does the opposite.

@bluwy Could you elaborate why this is the case? As far as I understand, targets is a browserlist query. If a modernTargets option were to be introduced, I would expect this also to be in browserlist query syntax and hence this needs to be converted to esbuild syntax to set modernTargetsEsbuild, doesn't it?

[bluwy]

@jgosmann If we introduce a new modernTargets option that's a browserlist query, then yes we can use the package to turn that into esbuild syntax. I was referring before that if we want re-use Vite's build.target config (esbuild syntax), then we'd need a way to turn that into browserlist.