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

Allow plugin-legacy to work for target with module support, but without dynamic import #3469

Closed
nulladdict opened this issue May 19, 2021 · 3 comments · Fixed by #3885
Closed
Labels
enhancement New feature or request plugin: legacy

Comments

@nulladdict
Copy link
Contributor

nulladdict commented May 19, 2021

Clear and concise description of the problem

2.3.0 bumped default build targets to dynamic import support and removed polyfill, which introduced some challenges in supporting browsers with modules but without dynamic imports (like edge18 #3388)
2.3.3 brought back the polyfill, but there are still some challenges present as described in this comment

It would be great, if legacy plugin allowed not polyfilling dynamic import (and subsequently not changing default build targets), but instead injected custom code before loading the main bundle, which would check support for the dynamic import, and load the fully-pollyfilled systemjs bundle in browsers, which support modules, but don't support the dynamic import

Suggested solution

Make legacy plugin do the following:

  • dot not inject dynamic import polyfill
  • detect dynamic import support before loading main "modern" bundle
  • if there's no support, instead load legacy "nomodule" bundle even if browser supports modules

Alternative

  • transpile main bundle to support edge18 and other browsers without dynamic import (as I understood correctly, there's some kind of issue with esbuild in this case)
  • hand-inject special loader code inside index.html after vite's build to check support and load legacy "nomodule" bundle. (this can't be done inside app entry, as main bundle would likely syntax error if dynamic import is used, rendering entire script useless)

Additional context

Seems like properly detecting dynamic import support requires eval, this worked for me in edge18:

function supportsDynamicImport() {
  try {
    return new Function(
      "return import('data:text/javascript;base64,Cg==').then(() => true)"
    )();
  } catch (e) {
    return Promise.resolve(false);
  }
} 
@patak-dev
Copy link
Member

@nulladdict I think it would be good if you would like to create a PR for this, so we can discuss with Evan having this option in the table.

About detecting dynamic import, we are already detecting it using eval here

self[importFunctionName] = new Function('u', `return import(u)`)

This is the detection scheme from https://github.com/GoogleChromeLabs/dynamic-import-polyfill

@nulladdict
Copy link
Contributor Author

Sure thing, I'll see what I can come up with and submit a PR
Let's discuss when the PoC is ready

@nulladdict
Copy link
Contributor Author

Hi, sorry for slacking on this

I've put together a PoC #3885, but I'm not really happy with how it turned out.
The solution seems kind of fragile to me and the other alternative would influence the main bundle, which is not ideal for the legacy feature in my opinion

I also checked which browsers are affected (supports es6-module and not supports es6-module-dynamic-import):

chrome 62
chrome 61
edge 18
edge 17
edge 16
firefox 66
firefox 65
firefox 64
firefox 63
firefox 62
firefox 61
firefox 60
ios_saf 10.3
opera 49
opera 48
safari 11
safari 10.1

To me, it looks like most of these apart from Edge and potentially Safari are auto-updatable, so the actual amount of affected users should be pretty low. Therefore it might be easier for users to upgrade than to have this feature implemented

@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request plugin: legacy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants