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

fix(plugin-legacy): legacy fallback for dynamic import #3885

Merged
merged 1 commit into from
Jul 23, 2021
Merged

fix(plugin-legacy): legacy fallback for dynamic import #3885

merged 1 commit into from
Jul 23, 2021

Conversation

nulladdict
Copy link
Contributor

@nulladdict nulladdict commented Jun 21, 2021

Closes #3469

Implementation checklist:

  • Add a new option to the plugin fallbackToLegacyIfNoDynamicImport (disabled by default)
  • Write a fallback script that loads SystemJs runtime and legacy bundle
  • Do not polyfill dynamic import if option is enabled
  • Inject dynamic import banner to ensure that modern bundle errors out as soon as possible
  • Investigate if it's possible to filter out syntax error from the modern bundle. (We don't want users or tools like Sentry to see this) Even if it's possible, there's no sure way to distinguish between import() syntax error and a regular SyntaxError, and we don't want to miss the later
  • Document new option, mention drawbacks and alternatives
  • Add CSP hash for the fallback script, add a note to the CSP section in the readme

Description

This is my attempt (or rather PoC) to implement the feature described in #3469.
I'm not quite happy with the current solution, but it's the best I could think of for now.

The problem is that I wanted for the dynamic fallback check to only influence the legacy bundle and not the main one, so proposed solution (check then load correct bundle) wouldn't work, since it requires code execution before even starting to download the correct bundle, while module/nomodule thing works at parse-time.

To not slow down the main bundle, I opted for this approach:

  • let tha main bundle download and execute (hoping that most of the targets actually have dynamic import)
  • in a separate module script check for dynamic import support and import polyfills and legacy chunk if there's none

The downsides are that main bundle would error-out if dynamic import syntax is not supported, which would produce error, which is not really desirable. Another potential problem is if the main bundle had no dynamic imports, it would fully execute and legacy bundle would load another copy of the app, which sounds like really bad news

Additional context


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.

@patak-dev
Copy link
Member

Hey @nulladdict, we discussed the different approaches with Evan and we think that we can proceed with this PR for the moment. There is a chance we may change the approach later in case we find a way to directly avoid downloading the modern bundle if it is not needed, but it may not end up happening because the percentage of users affected (0.49% currently) will continue to go down.

It looks like it is better to have the fallback as a separate script so we avoid having to include the hash of the legacy bundle in the modern bundle. This allows the user to update the targets for the legacy build, without affecting the modern bundle.

<script nomodule src="index-legacy.js"></script>
<script type="module">
  try {
    new Function('u', `return import(u)`);
  } catch () {
    // inject index-legacy.js
    // warning in the console about the dynamic import error
  }
</script>
<script type="module" src="index.js"></script>

In the modern bundle index.js inject a banner as you proposed (or the try/catch with the same guard if it works better)

import('data:text/javascript;base64,Cg==');

So the bundle is downloaded but it errors out without executing

Notes about drawbacks

  • The modern index.js module will always be downloaded
  • The user will see an error in the console. A global error handle could be added by the user to filter it?
    window.addEventListener('error', function(event) { ... })

What do you think?

@nulladdict
Copy link
Contributor Author

Ok, sounds good to me.
I'll add a checklist of what needs to be done and write an implementation.
I'm currently not sure how to inject a banner into the modern bundle. I guess I'll need to take a closer look at the code

@nulladdict nulladdict changed the title DRAFT: [plugin-legacy] legacy fallback for dynamic import [plugin-legacy] legacy fallback for dynamic import Jun 29, 2021
@nulladdict nulladdict marked this pull request as ready for review June 29, 2021 13:18
packages/plugin-legacy/README.md Outdated Show resolved Hide resolved
packages/plugin-legacy/index.js Show resolved Hide resolved
packages/plugin-legacy/index.js Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added plugin: legacy p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 3, 2021
@patak-dev patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 6, 2021
- Add a runtime check to fallback to legacy bundle
  for browsers with ESM but without dynamic import
- Stop enabling polyfillDynamicImport option

Closes #3469
@patak-dev
Copy link
Member

Looks good to me, thanks a lot for your work and patience to get to this point @nulladdict!
@AlexandreBonaventure, would you help us checking that your use case in #3388 is covered by this PR?

@AlexandreBonaventure
Copy link
Contributor

@patak-js sure no problem! and yeah thanks @nulladdict for putting all the work

@AlexandreBonaventure
Copy link
Contributor

So I reviewed a bit the PR, it it definetly looks like it will cover my use-case!

The one thing that could make it even better (for me at least), would be to have a way to opt-out modern bundle (and thus get rid of the drawbacks). In my case, I'm targeting a sandbox environment where I know dynamic import needs to be polyfilled anyways, so I'd like to disabled the modern bundle tag injection and basically just use Systemjs all the time. something like renderModernBundle: false ?
I can definetly help for adding this feature into the PR if you guys think that is a good idea.

@patak-dev
Copy link
Member

Thanks for reviewing this @AlexandreBonaventure! From your comment, I don't know if you had the opportunity to test the PR in your project, let us know if that is the case because it is very useful feedback.

About your proposed renderModernBundle: false feature, it is indeed interesting but it is out of the scope of this PR. I suggest you open a feature request for it explaining the use cases so we can discuss it with the team. A fair warning though is that we have already been adding a lot of options, so we should really justify introducing new complexity.

@AlexandreBonaventure
Copy link
Contributor

ok let me try in my project now then

@AlexandreBonaventure
Copy link
Contributor

@knightly-bot build this

@knightly-bot
Copy link

Nightly Build

🌒 Knightly build enabled, release every night at 00:00 UTC (skip if no change)

📦 npm package

@AlexandreBonaventure
Copy link
Contributor

OK @patak-js I can confirm it works perfectly with our project. Great job all !

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Code LGTM
We should notify community to try the next beta
One already confirmed: #3885 (comment), but we should await feedback from more community members with various setups

@patak-dev
Copy link
Member

This only affects plugin-legacy, so we dont need to wait for the next core beta. We could release this in a minor for the plugin

@Shinigami92
Copy link
Member

This only affects plugin-legacy, so we dont need to wait for the next core beta. We could release this in a minor for the plugin

Ah you are right 🙂

@patak-dev patak-dev changed the title [plugin-legacy] legacy fallback for dynamic import fix(plugin-legacy): legacy fallback for dynamic import Jul 23, 2021
@patak-dev patak-dev merged commit fc6d8f1 into vitejs:main Jul 23, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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