esbuild minify unexpect transform

[poyoho]

Describe the bug

when I use config for vite

export default {
build: {
    target: 'chrome60'
  }
}

esbuild transform dynamic import to require.

like this:

import('./workerImport-a2c311d3.js')

to

require('./workerImport-a2c311d3.js')

may be chrome63 support dynamic import. chrome60 no support. But I don't think this conversion meets the expectations.

I think need to polyfill import in chrome63

Reproduction

https://stackblitz.com/edit/node-qfc2yz?devtoolsheight=33&file=index.js

System Info

any

Used Package Manager

pnpm

Logs

No response

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 https://github.com/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.

[jplhomer]

But I don't think this conversion meets the expectations.

Can you describe what the expectations are?

[poyoho]

polyfill?

[poyoho]

I write a minirepo

https://stackblitz.com/edit/node-qfc2yz?devtoolsheight=33&file=index.js

[swandir]

If you are bundling an app, you might use @vitejs/plugin-legacy for that. It'll take care of dynamic imports.

For libraries, relying on Vite to handle dynamic imports, I believe, is not recommended.

Is build.target even intended for such use cases?

[bluwy]

With esbuild supporting the supported option, I tried the repro with:

    supported: {
      'dynamic-import': true,
    },

and it seems to fix the issue. Perhaps we should do something similar to #8665 (this PR affects optimizeDeps only)

[sapphi-red]

I think import needs to be preserved by esbuild, but it needs renamed in the final output. import is a keyword in js and to polyfill dynamic import, it needs to be a different name.
https://github.com/GoogleChromeLabs/dynamic-import-polyfill#:~:text=The%20name%20of%20the%20dynamic%20import%20polyfill%20function%20added%20to%20the%20global%20scope.

Note that renderDynamicImport/dynamicImportFunction cannot be used because Vite uses import in renderChunk by build-import-analysis which runs after them (#8588).

[bluwy]

I think polyfilling import would be the area of plugin-legacy which uses systemjs alltogether. In any case, preserving import seems more correct than using require, unless someone is polyfilling require for some reason, which they should've used plugin-legacy in the first place 🤔

[swandir]

I think polyfilling import would be the area of plugin-legacy

Sounds perfectly reasonable since dynamic imports are the new baseline for Vite

[sapphi-red]

I think polyfilling import would be the area of plugin-legacy

I think so too. But isn't preserving import confusing? If it does not work without plugin-legacy, I expect Vite to warn that.
That said, I think disabling import transformation will cause more understandable error (Unexpected token: import) than now (require is not defined). (Not sure if the error messages I wrote is same with the actual one.) So I'm for #9105.

[bluwy]

Yeah I think the error would be more obvious if we preserve it. And this also helps to make sure that the modern bundle can be used by modern browsers too. If we downlevel to require it'll break even on chrome100.

I tried to auto issue a warning too but I'm not sure if there's a neat way of doing so with esbuild.