search

ember-cli / ember-cli-babel-polyfills

Split-build polyfills for evergreen and legacy browsers
MIT License
34 stars 6 forks source link

Regenerator runtime is not loaded in modern browsers #14

Open andreyfel opened 3 years ago

andreyfel commented 3 years ago

This addon is supposed to include regenerator runtime into one of the polyfills and actually, it does include it in the legacy polyfill. However, the legacy polyfill is not loaded by modern browsers because of nomodule script attribute.

So, if the targets.js contains some legacy browsers which do not support async/await or generators the code gets transpiled by babel to use Regenerator runtime. So, when you run the app in a modern browser you end up in a situation when the code references regeneratorRuntime but the runtime is not there. Uncaught (in promise) ReferenceError: regeneratorRuntime is not defined.

Here is the reproduction repo: https://github.com/andreyfel/regenerator-runtime-bug

Most apps do not face the issue because ember-maybe-import-regenerator is in the default blueprint for Ember app. However, this addon is supposed to remove the need for that addon.

Also, ember-maybe-import-regenerator depends on a quite old version of the runtime. ember-cli-babel-polyfills has a much fresher version in dependencies.

I think the easiest fix for this would be to include regenerator runtime to the shared polyfill instead of the legacy one.

rwjblue commented 3 years ago

Hmm, I don't think that modern browsers need the regenerator runtime (e.g. when the apps targets include only browsers that are new enough not to need regenerator runtime) and it would be very unfortunate to force everyone to get it all the time (which is what would happen if we added it to shared).

andreyfel commented 3 years ago

@rwjblue If a legacy browser is targeted the code is transpiled to use regenerator runtime. So, even modern browsers need the regenerator runtime in this case. Take a look at the reproduction app and observe the crash in a modern browser.

rwjblue commented 3 years ago

Yep, I'm with you. But the solution isn't "always ship more stuff to everyone" 😉. The solution is to ship the regenerator runtime when your app is going to transpile to use it.

rwjblue commented 3 years ago

To be clear I'm happy to fix this here (and I do think we should!), but I think we'll have to be a bit smarter about how we include the runtime.

andreyfel commented 3 years ago

Totally agree!

rwjblue commented 3 years ago

I wonder if we can find out if regenerator-runtime will be used by the app 🤔

andreyfel commented 3 years ago

Maybe scan the app code processed through babel and see if it has regeneratorRuntime referenced anywhere?

Another option is to always include regenerator runtime if the target browsers do not support async/await or generators. The obvious downside is that if the app is not using async/await or generators and targets legacy browsers it will get the regenerator runtime anyway. But since Ember tests use async/await you are kind of forced to import regenerator runtime anyway if you are targeting legacy browsers. You can use ember-maybe-import-regenerator-for-testing which will import regenerator runtime only in tests, however, this approach is kind of error-prone since it masks the lack of regenerator runtime in dev/production: tests won't catch the lack of regenerator runtime when you introduce async/await to the app code.

So, IMO we should include regenerator runtime based on target browsers.