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

Struggling to polyfill dynamic import manually after upgrading to v2.3.0 #3388

Closed
5 tasks done
AlexandreBonaventure opened this issue May 12, 2021 · 12 comments
Closed
5 tasks done

Comments

@AlexandreBonaventure
Copy link
Contributor

Describe the bug

The latest minor release 2.3.0 dropped the built-in support for dynamic import polyfill. That's a shame for us bc we're developing an app with MSFT WebView 1 which uses Edge 18 under the hood. This browser falls in the category of support ESM but not dynamic import, so using the legacy plugin won't work.
We have to polyfill it ourselves now which is totally fine (we did that previously when using vite@1.x) but if you follow the guide here: https://github.com/antfu/vite-dynamic-import-polyfill-example you can see that it is using a renderDynamicImport rewrite technique which was discarded (here: e6f7fba#diff-e650901c71d775f0adc67ad233a9738d5322a3a5ad58155d709c77270dd32ff2L46) in the built-in plugin because it messes with the import analysis plugin. see https://github.com/vitejs/vite/pull/2976/files#diff-121079017d1fa98d6a0ea3354d5c73df45ab277078228cd7384c7a4e84c5a813L229

It means the migration path it not as easy as advertised bc of this issue, now that the polyfill is not built-in there is no way for us to hook into the build analysis plugin. So, we experience all sort of issues with css not loading and dynamic import relative path not being taken into account. (see antfu/vite-dynamic-import-polyfill-example#1 as well)

Reproduction

No reproduction for now, I'd like to gauge the stance of the team regarding this issue before committing too much time. I can provide if necessary. I wonder why this polyfill is not supported in the legacy plugin. As a user, if you need to target Edge 15 (no ESM support) you expect Edge 18 to work without a doubt, but here it won't work right ? (or am I missing something?)

Before submitting the issue, please make sure you do the following

  • 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.
  • Provide a description in this issue that describes the bug.
  • [ x 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/vue-next instead.
  • Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
@ludofischer
Copy link
Contributor

The latest minor release 2.3.0 dropped the built-in support for dynamic import polyfill. That's a shame for us bc we're developing an app with MSFT WebView 1 which uses Edge 18 under the hood. This browser falls in the category of support ESM but not dynamic import, so using the legacy plugin won't work.

Why won't the legacy plugin work? Are you seeing issues on Edge 18 with the legacy plugin or do you prefer the regular build for some reason?

I wonder why this polyfill is not supported in the legacy plugin.

The legacy plugin should convert ESM modules to SystemJS modules, so it should not need the import() polyfill.

@AlexandreBonaventure
Copy link
Contributor Author

@ludofischer because the legacy plugin is providing the SystemJS runtime only for browser that are not supporting ESM (ie: browsers that are going to run <script nomodule> tags)

@AlexandreBonaventure
Copy link
Contributor Author

if I could handle this with the legacy plugin with SystemJS, I'd be happy !

@AlexandreBonaventure
Copy link
Contributor Author

If for instance I would be able to prevent modern chunks distribution and just distribute the legacy chunks like without being wrapped in <script nomodule> it could work (via a option in legacy plugin, like legacyOnly: true)

@ludofischer
Copy link
Contributor

@ludofischer because the legacy plugin is providing the SystemJS runtime only for browser that are not supporting ESM (ie: browsers that are going to run <script nomodule> tags)

Ouch! You're right. The problem is that even bringing back the polyfill won't solve this, we would need to downgrade esbuild. The original problem that led to removing the polyfill was upgrading esbuild. We found if you use dynamic import, esbuild 0.11+ and rollup together (which Vite all combines under the hood) and the esbuild browser target includes edge18 (!!), it becomes very hard to output correct code (because esbuild turns import() into require() with no way to override it). We had to bump edge to at least 79, but then the polyfill became a bit useless, so we removed it.

@AlexandreBonaventure
Copy link
Contributor Author

hum yeah, that's a shame because to me that sounds like a esbuild bug. I understand the fix they did but it should only apply to a node environment... see https://esbuild.github.io/getting-started/#bundling-for-node. Otherwise it does not make any sense. Wouldn't it be easier to submit a PR to esbuild here and bring back the built-in polyfill ?

@ludofischer
Copy link
Contributor

If for instance I would be able to prevent modern chunks distribution and just distribute the legacy chunks like without being wrapped in <script nomodule> it could work (via a option in legacy plugin, like legacyOnly: true)

I wonder whether you couldn't remove the modern bundle already via a custom plugin and the transformIndexHtml hook.

Wouldn't it be easier to submit a PR to esbuild here and bring back the built-in polyfill ?

I suspect it is going to be faster to solve the issue at the Vite level. Even if someone makes a PR to esbuild, the maintainer needs to accept it, then merge it, esbuild needs to make a new release, Vite needs to bump esbuild and make a new release too, etc. And looking at the esbuild commit log, nobody regularly working on Vite ever had a commit merged into esbuild.

@AlexandreBonaventure
Copy link
Contributor Author

AlexandreBonaventure commented May 12, 2021

I wonder whether you couldn't remove the modern bundle already via a custom plugin and the transformIndexHtml hook.

Yes, probably can be worked-around for sure, while we figure out a better solution.
What do you guys think we should do moving forward ?

@ludofischer
Copy link
Contributor

Yes, probably can be worked-around for sure, while we figure out a better solution.
What do you guys think we should do moving forward ?

To be honest, I have only made one commit to Vite, so it's not like I am very representative of the main maintainers. From what you've told me about your situation, I would try to use https://vitejs.dev/guide/backend-integration.html and generate my own HTML without the modern bundle. If you use https://vitejs.dev/guide/api-plugin.html#transformindexhtml to remove the modern bundle, it looks like you receive the existing HTML as a string, so in that case the only way forward is some search/replace on that string, which could be brittle.
Legacy mode only on the legacy plugin sounds like something that could interest others too.

@patak-dev
Copy link
Member

Thanks for this discussion and sorry for the disruption with this use case in v2.3.0. @AlexandreBonaventure please check out #3434, released as part of Vite v2.3.3.

You should be able to set in your vite.config.js build.polyfillDynamicImport to true to get back the dynamic import polyfill. @vitejs/plugin-legacy is going to set this config on by default so that is another option to resolve this problem. Let us know if this works correctly.

@AlexandreBonaventure
Copy link
Contributor Author

@patak-js Awesome! thanks for reacting so quickly, it works gangbusters!

@AlexandreBonaventure
Copy link
Contributor Author

AlexandreBonaventure commented May 18, 2021

@patak-js following up, and as an fyi: There is still a general issue that makes supporting edge18 in particularly difficult because of #3388 (comment)

Meaning that now since you guys restored the dynamic polyfill we can use it BUT, all the other polyfills/es syntax won't be transpiled (still because the legacy plugin target non ESM browsers). What I've been using esbuild: { target: 'edge18} won't work anymore because of evanw/esbuild#1281. To me that's more an esbuild problem but all in all my comment here still stands: #3388 (comment)

Of course, I could build a custom config/plugin for vite to do that, but that's a lot of work for reinventing the wheel

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants