Add noConflict entry mode to @babel/polyfill #6371

evan-scott-zocdoc · 2017-10-03T02:56:08Z

Q	A
Fixed Issues	Fixes #4019
Patch: Bug Fix?	no
Major: Breaking Change?	no
Minor: New Feature?	no
Tests Added/Pass?
Spec Compliancy?
License	MIT
Doc PR
Any Dependency Changes?

Currently if babel-polyfill is used twice on the same page, a hard error is emitted. This is unfortunate from a developer experience perspective because we can't always be in control of what is being loaded by library authors or other packages.

Assuming loading polyfill twice doesn't actually break anything, a warning would be much more friendly and avoid breaking build systems that halt on JS errors when the application remains functional.

babel-bot · 2017-10-03T03:04:39Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5768/

hmbenner · 2017-10-31T17:54:21Z

Came to make this exact change, is there anything preventing this from being incorporated in the next patch version?

If the merge conflict is the only blocker, I'd be happy to re-fork and make this change against the latest version.

hmbenner · 2017-10-31T17:56:42Z

Actually, does it makes sense to prevent further execution if an existing global._babelPolyfill is found?

evan-scott-zocdoc · 2017-11-01T04:50:34Z

@jbenner rebased and adjusted the implementation

loganfsmyth · 2017-11-01T05:02:03Z

The title of this says it will show a warning, but you don't appear to have that in the code. The warning is specific because the polyfill is global, so if you're loading multiple copies you, or something else on the page with you, is likely doing something wrong. I'm fine with it not being a hard error, but it should at least have a warning for users since 90% of the time this will be a bug, even if it isn't 100% of the time.

evan-scott-zocdoc · 2017-11-01T05:05:26Z

@loganfsmyth that was the original implementation, sorry for the confusion. I agree with @jbenner that simply not applying the polyfill more than once by checking for the global flag is a more elegant solution.

If you'd like I can definitely also add a warning, but this behavioral revision ~~eliminates~~ prevents the root problem of accidentally applying the polyfill multiple times.

hmbenner · 2017-11-01T15:24:31Z

Nice - I think a warning if global._babelPolyfill === true when babel-polyfill is loaded is helpful. The console.warn makes sense, similar to what's seen when minifying the development version of React (a gentle prodding that implies "are you sure this is what you meant to do?").

Thanks for updating this!

loganfsmyth · 2017-11-01T17:03:40Z

@evan-scott-zocdoc The issue is that babel-polyfill is global. There is no guarantee that the version of it that is detected is the same version you're loading. What if the version you're loading is newer and has more stuff that you rely on being there that the old version doesn't have? What if the version of the regenerator runtime is different and breaks because it got replaced with a different version? You don't know which version of the polyfill logic is the one you actually want. Even then, depending on what has changed you might actually need both, hence why until now we've thrown hard when two copies are detected.

If we're not going to throw, we absolutely at least need to keep running the polyfill both times, with a warning for users.

The reason this warning exists is because babel-polyfill is only recommended for applications that "own" the content of the page. If you're writing a widget that embeds into a page but doesn't own it, babel-polyfill isn't what you should be using in the first place.

evan-scott-zocdoc · 2017-11-01T17:06:53Z

@loganfsmyth Perhaps the guard should be discarded then and split apart into separate guards for each of the polyfills.

The specific use case where this error becomes very annoying is when you do own a page and you're injecting multiple of your own bundles onto it in various combinations.

loganfsmyth · 2017-11-01T17:12:53Z

Have you explored using Webpack's chunking plugins? Usually I'd expect that to be configured so even when you eventually load multiple pages of an app, the common stuff only loads once. Otherwise you're also wasting tons of bandwidth redownloading something you already got.

evan-scott-zocdoc · 2017-11-01T17:17:09Z

@loganfsmyth yeah, we have a mixed environment so this sort of stuff becomes very challenging, unfortunately.

loganfsmyth · 2017-11-01T17:31:59Z

There's nothing stopping you from doing global._babelPolyfill = false; require('babel-polyfill'); in your own code to disable this warning entirely. My concern is just with the default behavior of this module. The warning is there because in a high percentage of cases if this is being loaded twice, it's a mistake on the user's part.

hmbenner · 2017-11-01T20:56:33Z

We currently use Webpack's chunking plugin to extract shared libraries to a common file, but have an ecosystem where not all developers are integrated into our unified frontend build (yet). Much of the desire behind this is to avoid careful release timing between non-integrated applications that reside in the shared space.

It would seem that a developer using global._babelPolyfill = false; require('babel-polyfill'); is deliberately ignoring/sidestepping the throw that exists today. In that case it seems reasonable to expect that individual is implementing a non-happy path approach and knows what they're doing (or possibly, isn't aware and should remove the workaround?)

Can you help me understand the use case where it's valid to import babel-polyfill multiple times at different versions and expect them to coexist happily, without additional work? If I am picturing this correctly, changing from a throw to a console.warn would support that use case, whereas today it would be a manual sidestep a la global._babelPolyfill = false; require('babel-polyfill');

I do admit that a if (!global.__babelPolyfill) require('babel-polyfill'); workaround solves our specific issue, and is the approach we are currently taking. However, I found enough instances of individuals running into this issue that it seemed logical to advocate for a solution at the library level. I completely understand if this is contradictory to the ideology of the library.

loganfsmyth · 2017-11-01T22:54:25Z

It would seem that a developer using global._babelPolyfill = false; require('babel-polyfill'); is deliberately ignoring/sidestepping the throw that exists today. In that case it seems reasonable to expect that individual is implementing a non-happy path approach and knows what they're doing (or possibly, isn't aware and should remove the workaround?)

Totally agree. I was suggesting it because I'd expect most of the people running into this today who want to work around it fall into that non-happy path.

Can you help me understand the use case where it's valid to import babel-polyfill multiple times at different versions and expect them to coexist happily, without additional work? If I am picturing this correctly, changing from a throw to a console.warn would support that use case, whereas today it would be a manual sidestep a la global._babelPolyfill = false; require('babel-polyfill');

The polyfills will happily coexist. The regenerator runtime is the tough part, but I also don't know of any expected changes to it, so it's kind of a toss-up, so realistically I see no reason to throw. The goal is 100% to say "hey you're downloading two copies of the polyfill and that's probably an accident", so changing it to a warning, and exposing a way to explicitly opt out of the warning seems fine to me.

I do admit that a if (!global.__babelPolyfill) require('babel-polyfill'); workaround solves our specific issue, and is the approach we are currently taking. However, I found enough instances of individuals running into this issue that it seemed logical to advocate for a solution at the library level. I completely understand if this is contradictory to the ideology of the library.

Maybe we can expose a

require("babel-polyfill").noConflict()

that would set the global flag then unset it with the noConflict() call, you can basically say "if this gets loaded again, don't worry about it?

evan-scott-zocdoc · 2017-11-01T23:03:08Z

require("babel-polyfill").noConflict()

Actually that would work wonderfully. Especially if you could just import it like

import 'babel-polyfill/noConflict';

evan-scott-zocdoc · 2017-11-01T23:03:57Z

I could see that getting complicated though when used in conjunction with babel-preset-env, since it replaces the import call with individual ones when "usage" mode is enabled.

loganfsmyth · 2017-11-01T23:09:18Z

Fair. We could pretty easily make preset-env detect both though.

richard-engineering · 2017-11-02T18:56:40Z

How would noconflict work with webpack when used in entry? would it just be:

'entry': [
   'test': ['babel-polyfill/noConflict', srcdir + '/test/app.jsx'],
]

evan-scott-zocdoc · 2017-11-02T18:59:02Z

@rhan-mentad yeah exactly

evan-scott-zocdoc · 2017-11-02T23:55:47Z

@loganfsmyth Revised, let me know what you think!

evan-scott-zocdoc · 2017-11-02T23:56:38Z

packages/babel-polyfill/src/index.js

 global._babelPolyfill = true;

-import "core-js/shim";
-import "regenerator-runtime/runtime";
+require("core-js/shim");


I changed these to require since import by spec has to be at the top of the file, right?

How do you feel about ordering? Most frameworks would do this logic as essentially

require("core-js/shim"); require("regenerator-runtime/runtime"); if (global._babelPolyfill) { throw new Error( "@babel/polyfill is loaded more than once on this page. This is probably not desirable/intended and may have consequences if different versions of the polyfills are applied sequentially. If you do need to load the polyfill more than once, use @babel/polyfill/noConflict instead to bypass the error.", ); } global._babelPolyfill = true; modue.exports = function(){ global._babelPolyfill = false; };

with /noConflict just doing

require("./index").noConflict();

so if something loaded first in non-conflict mode, it'd error, but you can explicitly say that the version you are loading should not conflict with things loaded later.

loganfsmyth · 2017-11-03T00:24:21Z

.eslintrc

@@ -6,7 +6,8 @@
  "rules": {
    "curly": ["error", "multi-line"],
    "prettier/prettier": ["error", { "trailingComma": "es5" }],
-    "no-case-declarations": "error"
+    "no-case-declarations": "error",
+    "max-len": ["error", { "ignoreStrings": true }]


I'd just wrap the string with concatenation.

Okay :( just felt gross

xtuc

Thanks for your PR.

LGTM if that can fix your issue.

evan-scott-zocdoc · 2018-03-23T14:40:08Z

@xtuc can this be merged?

Andarist · 2018-03-23T15:01:04Z

Isn't it quite different from what noConflict usually do? Normally it restores previous value - which we won't be able to do in case of babel-polyfill

evan-scott-zocdoc · 2018-03-23T15:02:35Z

What would you name it then?

Andarist · 2018-03-25T10:35:10Z

allowDuplicate? not the best, but 🤷‍♂️

arfordweb · 2018-03-29T22:56:01Z

So, why is noConflict preferable to changing the hard throw to a console.error or console.warn? If the developer sees in the console that more than one polyfill is being loaded, but everything seems to work ok, they can ignore the console message until they have time to delve into why the polyfill is loaded twice.

That:

gives them incentive to fix the issue

But:

doesn't stop them from releasing
doesn't force them to work around the issue by conditionally requiring this one package when all their other dependencies are imported with ES6 static imports

quantizor · 2018-03-30T06:48:51Z

@arfordweb for what it’s worth, I 100% agree with making this a console error or warning rather than a hard one and that was even the original PR before code review

yvele · 2018-04-03T18:23:51Z

As @evan-scott-zocdoc mentioned in his comment, will this PR manage multiple polyfills bundled with different babel-preset-env settings? Here is my use case #4019 (comment)

PS: Would it be nice to add some guidelines to babel doc for developers releasing Web libraries? (like bundling 2 libraries, one with polyfills and another one without any polyfill.. I duno.. 🤷‍♂️)

evan-scott-zocdoc · 2018-04-03T18:37:25Z

@yvele they'd still clobber each other, the way we compile code via babel would need to change a lot to overcome this :/

hzoo · 2018-04-21T15:57:22Z

packages/babel-polyfill/src/index.js

+  console.warn(
+    "@babel/polyfill is loaded more than once on this page. This is probably not desirable/intended " +
+      "and may have consequences if different versions of the polyfills are applied sequentially. " +
+      "If you do need to load the polyfill more than once, use @babel/polyfill/noConflict " +


I think this has to be @babel/polyfill/lib/noConflict right? since it's not the main file.

loganfsmyth · 2018-04-21T16:47:05Z

packages/babel-polyfill/src/index.js

@@ -1,7 +1,13 @@
+import "core-js/shim";
+import "regenerator-runtime/runtime";
+
 if (global._babelPolyfill) {


Should we do

if (global._babelPolyfill && typeof console !== "undefined" && console.warn) {

so that this doesn't throw on platforms that don't have console?

loganfsmyth · 2018-04-21T16:51:19Z

packages/babel-polyfill/src/index.js

+  console.warn(
+    "@babel/polyfill is loaded more than once on this page. This is probably not desirable/intended " +
+      "and may have consequences if different versions of the polyfills are applied sequentially. " +
+      "If you do need to load the polyfill more than once, use @babel/polyfill/lib/noConflict " +


I think if we want to expose this, we should make a noConflict in the root that proxies through. I don't like people even knowing lib exists really.

Ok for sure I don't like the lib thing either

loganfsmyth · 2018-04-22T23:08:34Z

packages/babel-polyfill/noConflict.js

@@ -0,0 +1 @@
+import "lib/noConflict";


This would need to be require :)

and ./lib/, not lib/

evan-scott-zocdoc force-pushed the es-remove-polyfill-err branch 2 times, most recently from d6d1f9b to 1504a91 Compare November 1, 2017 04:50

evan-scott-zocdoc changed the title ~~hard error -> warning for overlapping babel-polyfill usage~~ eliminate error for using babel-polyfill more than once Nov 1, 2017

evan-scott-zocdoc force-pushed the es-remove-polyfill-err branch from 1504a91 to a580c2d Compare November 2, 2017 23:53

evan-scott-zocdoc changed the title ~~eliminate error for using babel-polyfill more than once~~ Add noConflict entry mode to @babel/polyfill Nov 2, 2017

evan-scott-zocdoc force-pushed the es-remove-polyfill-err branch from a580c2d to 4fa188d Compare November 2, 2017 23:54

evan-scott-zocdoc commented Nov 2, 2017

View reviewed changes

loganfsmyth reviewed Nov 3, 2017

View reviewed changes

feat: add noConflict polyfill entry

1f01a5c

evan-scott-zocdoc force-pushed the es-remove-polyfill-err branch from c8c0988 to 1f01a5c Compare November 10, 2017 23:19

xtuc approved these changes Nov 16, 2017

View reviewed changes

xtuc mentioned this pull request Apr 3, 2018

Manage multiple babel-polyfill imports instead of throwing an exception (T7013) #4019

Closed

hzoo added 2 commits April 21, 2018 11:54

use a warn [skip ci]

f3ac0f9

import directly [skip ci]

565c097

hzoo reviewed Apr 21, 2018

View reviewed changes

update message [skip ci]

f383120

loganfsmyth reviewed Apr 21, 2018

View reviewed changes

hzoo added 2 commits April 21, 2018 14:35

add console check [skip ci]

5cb44c2

add entry point [skip ci]

72210ae

hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Apr 21, 2018

loganfsmyth reviewed Apr 22, 2018

View reviewed changes

not compiled [skip ci]

7c00ed8

loganfsmyth approved these changes Apr 22, 2018

View reviewed changes

hzoo merged commit 0bb71ca into babel:master Apr 22, 2018

yvele mentioned this pull request Sep 19, 2018

New @babel/polyfill package using Babel 7 (embedded noConflict mode) clayrisser/idempotent-babel-polyfill#2

Closed

mateoclarke mentioned this pull request Oct 13, 2018

Residential Parking Form (out of box Knack.js form embed) cityofaustin/janis#271

Closed

apwidejulien mentioned this pull request May 28, 2019

Avoid babel polyfill collision when loaded multiple times by differen… yotamberk/timeline-plus#164

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Add noConflict entry mode to @babel/polyfill #6371

Add noConflict entry mode to @babel/polyfill #6371

Conversation

evan-scott-zocdoc commented Oct 3, 2017 • edited Loading

babel-bot commented Oct 3, 2017 • edited Loading

hmbenner commented Oct 31, 2017 • edited Loading

hmbenner commented Oct 31, 2017

evan-scott-zocdoc commented Nov 1, 2017

loganfsmyth commented Nov 1, 2017

evan-scott-zocdoc commented Nov 1, 2017 • edited Loading

hmbenner commented Nov 1, 2017

loganfsmyth commented Nov 1, 2017

evan-scott-zocdoc commented Nov 1, 2017 • edited Loading

loganfsmyth commented Nov 1, 2017

evan-scott-zocdoc commented Nov 1, 2017

loganfsmyth commented Nov 1, 2017

hmbenner commented Nov 1, 2017

loganfsmyth commented Nov 1, 2017

evan-scott-zocdoc commented Nov 1, 2017

evan-scott-zocdoc commented Nov 1, 2017

loganfsmyth commented Nov 1, 2017

richard-engineering commented Nov 2, 2017 • edited Loading

evan-scott-zocdoc commented Nov 2, 2017

evan-scott-zocdoc commented Nov 2, 2017

Choose a reason for hiding this comment

loganfsmyth Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc left a comment

Choose a reason for hiding this comment

evan-scott-zocdoc commented Mar 23, 2018

Andarist commented Mar 23, 2018

evan-scott-zocdoc commented Mar 23, 2018

Andarist commented Mar 25, 2018

arfordweb commented Mar 29, 2018 • edited Loading

quantizor commented Mar 30, 2018 • edited Loading

yvele commented Apr 3, 2018 • edited Loading

evan-scott-zocdoc commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-scott-zocdoc commented Oct 3, 2017 •

edited

Loading

babel-bot commented Oct 3, 2017 •

edited

Loading

hmbenner commented Oct 31, 2017 •

edited

Loading

evan-scott-zocdoc commented Nov 1, 2017 •

edited

Loading

evan-scott-zocdoc commented Nov 1, 2017 •

edited

Loading

richard-engineering commented Nov 2, 2017 •

edited

Loading

loganfsmyth Nov 3, 2017 •

edited

Loading

arfordweb commented Mar 29, 2018 •

edited

Loading

quantizor commented Mar 30, 2018 •

edited

Loading

yvele commented Apr 3, 2018 •

edited

Loading