-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add support for extending builtins #7020
Conversation
|
||
This transformer works on IE11 and every other browser with `Object.setPrototypeOf` or `__proto__` as fallback. | ||
|
||
There is **NO IE <= 10 support**. If you need IE <= 10 don't it's recommended to not don't extend natives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, I made it super clear there 😂
If you need IE <= 10 don't it's recommended to not don't extend natives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the don't use this plugin
bit and screwed it up 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that IE is not official supported by Babel (according to https://github.com/babel/babel/blob/master/doc/design/compiler-environment-support.md)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xtuc You mean that the compiler doesn't work IE, don't you? I surely hope Babel itself still supports IE...
|
||
This transformer works on IE11 and every other browser with `Object.setPrototypeOf` or `__proto__` as fallback. | ||
|
||
There is **NO IE <= 10 support**. If you need IE <= 10 don't it's recommended to not don't extend natives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grammar "to not don't"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds strange to me, could we reword it to something like:
We don't recommend to extend native classes under IE (version <= 10) because it can cause undefined behaviors.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6204/ |
I have not caught up with the discussion in the other thread - what's the downside of doing inheritance buble-style? var MyError = (function (Error) {
function MyError () {
Error.apply(this, arguments);
}if ( Error ) MyError.__proto__ = Error;
MyError.prototype = Object.create( Error && Error.prototype );
MyError.prototype.constructor = MyError;
return MyError;
}(Error)) Maybe such simpler transform could be used in |
We already changed the loose mode transform to be simpler? #4850 unless you mean even more so |
@Andarist Some builtins (like
|
@Andarist many constructors won't work there.
|
@nicolo-ribaudo the issue in the test is probably here. Checking I think that |
@nicolo-ribaudo you also flipped the wrap, targeting directly the builtin instead of the declared class. I think that's OK but then you don't need a That means that using |
"Float32Array", | ||
"Float64Array", | ||
"Function", | ||
"In16Array", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
"Number", | ||
"Object", | ||
"Promise", | ||
"Proxy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth it to include this here when Proxy
can't get subclassed anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes any sense to maintain this list manually.
This is what I've used to include them all, something you can produce on any browser via:
JSON.stringify(
Object.getOwnPropertyNames(window)
.filter(name => /^[A-Z]/.test(name))
.sort(),
null,
' '
)
so that from time to time one can run that again and update the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially did something like this, but then it would include things like Math
, Infinity
...
I will ask to include this list in the Globals package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that filter might include typeof “function” too
Thank you all for the quick feedback, they are really appreciated. I'll update the PR tomorrow. |
var _cache = typeof WeakMap === "function" && new WeakMap(); | ||
|
||
export default function _wrapNativeSuper(Class) { | ||
if (_cache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this check be more explicit? typeof x === 'undefined'
|
||
This transformer works on IE11 and every other browser with `Object.setPrototypeOf` or `__proto__` as fallback. | ||
|
||
There is **NO IE <= 10 support**. If you need IE <= 10 don't it's recommended to not don't extend natives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that IE is not official supported by Babel (according to https://github.com/babel/babel/blob/master/doc/design/compiler-environment-support.md)
helpers.wrapNativeSuper = defineHelper(` | ||
var _gPO = Object.getPrototypeOf || function _gPO(o) { return o.__proto__ }; | ||
var _sPO = Object.setPrototypeOf || function _sPO(o, p) { o.__proto__ = p }; | ||
var _construct = Reflect.construct || function _construct(Parent, args, Class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check for Reflect first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, and original plugin was doing it indeed:
https://github.com/WebReflection/babel-plugin-transform-builtin-classes/blob/master/src/index.js#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use (typeof Reflect === "object" && Reflect.construct) || function ...
instead of the ternary (which is used in the original plugin) since someone might only polyfill some Reflect methods, so Reflect.constructor
could be not defined.
|
||
var _cache = typeof WeakMap === "function" && new WeakMap(); | ||
|
||
function _wrapNativeSuper(Class) { if (_cache) { if (_cache.has(Class)) return _cache.get(Class); _cache.set(Class, Wrapper); } function Wrapper() {} Wrapper.prototype = Object.create(Class.prototype, { constructor: { value: Wrapper, enumerable: false, writeable: true, configurable: true } }); return _sPO(Wrapper, _sPO(function Super() { return _construct(Class, arguments, _gPO(this).constructor); }, Class)); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need enumerate: false
(default, and if omitted, core-js/es5-sham can avoid being forced to throw on ES3 engines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true that it's false
by default but also true that ES3 engines should throw if you extend builtins because ES3 engines are not supported. IE <= 10 is discouraged and that's already mostly ES5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enumerable: false
is also used by the _inherits
helper.
Also, ES3 engines don't support setPrototypeOf
/__proto__
, so classes won't work in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of __proto__
was actually added to the helpers specifically so it would work on ES3 engines that were polyfilled.
The idea was that, even if the engine doesn't support it, as long as we read from __proto__
first, we're going to get a reference to the relevant constructor, which is all that we need for calling super. static property inheritance will still, of course, be broken.
@@ -9,7 9,7 @@ export default new Set([ | |||
"Float32Array", | |||
"Float64Array", | |||
"Function", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure having at least HTMLElement
in this list by default would make many people happy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with it, but I'd like to wait for other opinions (since currently Babel is only about ECMAScript, and not about DOM extensions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a reasonable exception to the rule, specially considering Babel is used the most for the Web.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add Event
and CustomEvent
. Considering that these classes aren't wrapped until used, this doesn't seem to add more cost to the transform. I think this transform will be most popular with web components users, so having a useful default seems better to me.
`Array<string>`, defaults to `[]`. | ||
|
||
When extending a native constructor (e.g., `class extends Array {}`), it needs | ||
to be wrapped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of changes I'd reccomend:
- Change "constructor" to "class" (since the key distinction here is constructors that can't be called with
Function.apply
, which are class constructors) and mention a little about w - Clarify what needs to be wrapped: the subclass or the native class
- Add just a little hint on why the wrapper is necessary
"When extending a native class (e.g., class extends Array {}
), the native class needs to be wrapped in order to support Babel's emulation of super()
calls."
|
||
When extending a native constructor (e.g., `class extends Array {}`), it needs | ||
to be wrapped. | ||
Babel only recognizes built-in JavaScript functions defined in the ECMAScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this option override or augment the default list? If it augments, I'd consider making that more clear in the option name, like additionalBuiltins
.
const { name } = this.superName; | ||
this.extendsNative = | ||
this.isDerived && | ||
(builtinClasses.has(name) || customBuiltins.has(name)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the extension of the native is indirect due to a mixin application? ie:
class MyElement extends PolymerElement(HTMLElement) {}
Can we have a way to force the wrapping of a particular set of builtins? It'd be nice to have a test for the mixin case too, considering how popular the pattern is in the Web Components ecosystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an easy work around could be to:
class MyElement extends PolymerElement(class extends HTMLElement {}) {}
I don't think it's that simple to handle every time a constructor is passed around and in a meaningful way.
mixins also can be passed around as regular invoke.
I mean ... this is valid code too, right?
const mixed = (E => () => PolymerElement(E))(window['HTMLElement']);
class MyElement extends mixed() {}
We need a compromise for common cases and IMO these are not necessarily based on Polymer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most common case in the wild is that HTMLElement
is extended via mixin application. Without support for that, this transform will be much less used than it could be.
This is why I suggest a way to force wrapping, possibly like:
{
"plugins": [
["@babel/transform-classes", {
"builtins": [ "HTMLElement" ],
"forceWrap": [ "HTMLElement" ],
}]
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most common case in the wild is that HTMLElement is extended via mixin application.
I'd like to see data about this. I use CE since ever and never used mixins there. It's also worth noticing that Babel supposed to be about ES, not DOM, as previously mentioned.
Every extended builtin constructor is broken right now so this is already quite a big step forward.
This is why I suggest a way to force wrapping, possibly like:
I'm a bit confused by the fact you don't want to double wrap polyfills for Custom Elements but you want to force wrap HTMLElement
which basically means the transpiled code will wrap already that class ...
how does it work? You either force wrap, or you avoid runtime, when happens, the double wrap.
Unless you are thinking about having the polyfill before the transpiled code that uses that constructor ... is that the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polyfill would load before the compiled code, and so would wrap native HTMLElement to have a working constructor in the first place, otherwise without the polyfill both new HTMLElement()
and HTMLElement.apply
fail. So when the polyfill is loaded, the wrapping doesn't need to happen.
Optimally, code would be compiled to "force" wrapping of HTMLElement, even if it can't statically see a direct subclass, but the wrapping would bail if the constructor is callable.
var _cache = typeof Map === "function" && new Map(); | ||
|
||
export default function _wrapNativeSuper(Class) { | ||
if (typeof _cache !== "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be really nice if this didn't wrap emulated classes which already have callable constructors, like when using the Custom Elements polyfill. In that case HTMLElement
is callable because the polyfill wraps the native class. Detecting this case would avoid double-wrapping.
It's unfortunately impossible to absolutely detect that a constructor function is callable, but there's an approximation that might be suitable here:
function isNative(ctor) {
return ctor.toString().indexOf('[native code]') !== -1;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also catch user-defined classes, but one could then question whether you actually need this transform if you expect to find such a class:
function isNative(ctor) {
return /^class|\[native code\]/.test(ctor.toString())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to catch user defined classes with a function called isNative
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't work, because core-js makes methods "look native". e.g.
https://github.com/zloirock/core-js/blob/16357e4fefa070573a342ef2a5692f30ba4320c6/tests/tests/es6.promise.ls#L9
https://github.com/zloirock/core-js/blob/6e389278357a6296a58155742871ca876dff2bc3/tests/helpers/looks-native.ls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are here and there Babel specific conventions, like __esModule = true
, I wonder if it's worth considering a non enumerable, non configurable, __nativeWrap = true
to explicitly opt out from Babel wraps so that the check would be builtins.includes(ctor.name) && !ctor.__nativeWrap
Simple enough to cover polyfills case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kovensky:
This would also catch user-defined classes, but one could then question whether you actually need this transform if you expect to find such a class
It happens, especially in the case where a library is published compiled to npm, and used by an app that's shipping native ES6. We try to discourage distributing compiled code, but we can't control that.
We could indeed add a flag to the polyfill. What we really want is this: https://esdiscuss.org/topic/add-reflect-isconstructor-and-reflect-iscallable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this discussion caan be deferred, since it works as it is implemented in this PR.
Don't wrap classess without necessity is a possible performance optimization, but for now shipping a working implementation is enough.
a.push.apply(a, args); | ||
Constructor = Parent.bind.apply(Parent, a); | ||
return _sPO(new Constructor, Class.prototype); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely outside the scope of this PR, but I'd actually prefer to have the second argument to the regular _possibleConstructorReturn
call replaced with a call to a helper that's something like:
function _constructInstance (Constructor, args, newTarget, existingNewTarget) {
_constructInstance = typeof Reflect !== "undefined" && Reflect.construct
? Reflect.construct
: _constructWithApply;
return _constructInstance.apply(this, arguments);
function _constructWithApply (Constructor, args, newTarget, existingNewTarget) {
return Constructor.apply(existingNewTarget, args);
}
}
That is a much simpler version of this wrapper, which would still not support extending builtins unless your browser supports Reflect.construct
. This cheats by giving a 4th (ignored) argument to Reflect.construct
, but otherwise replicates the current behavior (without this PR) when _constructWithApply
is used instead.
A _possibleConstructorReturn
call would then look like:
_possibleConstructorReturn(this,
_constructInstance(Derived.__proto__ || Object.getPrototypeOf(Derived), arguments, this.constructor || Derived, this)
);
This does depend on this.constructor
not being tampered with, though; the same caveats as the new.target
transform (what the third argument actually is). Giving Derived
itself as the third argument would make Derived
not subclassable.
This also comes with a performance hit on engines old enough to need this transform 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely outside the scope of this PR
what you say makes sense but please let's not block the shipping of this PR introducing extra, not directly related, discussions, thanks
var _cache = typeof Map === "function" && new Map(); | ||
|
||
export default function _wrapNativeSuper(Class) { | ||
if (typeof _cache !== "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also catch user-defined classes, but one could then question whether you actually need this transform if you expect to find such a class:
function isNative(ctor) {
return /^class|\[native code\]/.test(ctor.toString())
}
const Constructor = loose ? LooseTransformer : VanillaTransformer; | ||
|
||
let customBuiltins; | ||
if (builtins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd check for builtins !== undefined
, but maybe it's intended that you can pass builtins: false
?
(is it also intended that you can pass 0
or ""
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be !== undefined
@@ -61,7 63,7 @@ const findThisesVisitor = traverse.visitors.merge([ | |||
]); | |||
|
|||
export default class ClassTransformer { | |||
constructor(path: NodePath, file) { | |||
constructor(path: NodePath, file, customBuiltins: Set<string>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does flow have a ReadonlySet
type? I'd prefer to use that if possible since it'd guard against unintentional modification (Set
minus all methods that cause mutation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the only read-only type is ReadonlyAray
@@ -425,6 425,47 @@ helpers.inheritsLoose = defineHelper(` | |||
} | |||
`); | |||
|
|||
// Based on https://github.com/WebReflection/babel-plugin-transform-builtin-classes | |||
helpers.wrapNativeSuper = defineHelper(` | |||
var _gPO = Object.getPrototypeOf || function _gPO(o) { return o.__proto__ }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the workaround in #3527 but there's no hope of extending builtins working if the workaround is necessary anyway 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this patch is explicitly incompatible with IE <= 10 where you should not extend native builtins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, if we're writing new code that extends builtins...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you are responsible to make such code work as expected otherwise you should stop using JavaScript since even Object
could be overwritten 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, let me stop using JS on my next web app!
Currently this PR only wraps globals. After thinking a bit about it, I come to the conclusion that classes defined by the |
@nicolo-ribaudo but isn't the whole purpose of this wrap to fix only the builtins extend? AFAIK there are no private, or user defined classes, that need such work around, right? |
You can import an un-transpiled class from an extrnal npm module and extend it with a transpiled class. Also, I opened sindresorhus/globals#126 to remove the list of builtins in this PR. If that issue gets accepted, it probably would also handle every browser builtin. |
that list won't include |
@WebReflection Yes. It could be an option (e.g. |
@WebReflection 46325e1 enables wrapping for DOM classes always, since in non-browsers users won't use them anyway. |
I would still like to see this comment about forcing the wrapping of certain native classes, for when we can't statically determine that they're subclassed: #7020 (comment) LGTM otherwise! |
Sounds like that could be a good follow-up pr/discussion? Making an option if it will be a common thing sounds fine to me, or would you like to make an issue to track @justinfagnani? |
Nice work on following through with this @nicolo-ribaudo and everyone's reviews - good community effort to move this forward finally 😄 |
🎉 |
// Based on https://github.com/WebReflection/babel-plugin-transform-builtin-classes | ||
helpers.wrapNativeSuper = defineHelper(` | ||
var _gPO = Object.getPrototypeOf || function _gPO(o) { return o.__proto__ }; | ||
var _sPO = Object.setPrototypeOf || function _sPO(o, p) { o.__proto__ = p }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug! This fallback does not return o
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that this hasn't been fixed in this PR, but it looks like it's fixed now.
How do I use this? I'm in Meteor 1.6.1 which uses Babel 7 @nicolo-ribaudo I read in the OP,
What does that mean exactly? Can you provide an example? I get an error like this:
Where my app code is like this: customElements.define('my-el', class MyEl extends HTMLElement {
connectedCallback() {
console.log(' ------ my el!!')
}
})
document.body.appendChild(document.createElement('my-el')) |
I checked the source, and I see that What |
I made a simple reproduction (using just Babel, no Meteor) showing that I'm using https://github.com/trusktr/babel/tree/issue-7020 It includes the built output in You can also try Did I miss some configuration, or is there a bug? |
globals
toplugin-transform-classes
, but it should already be in users' node_modules folders because it is used by@babel/traverse
This PR adds the helper used by https://github.com/WebReflection/babel-plugin-transform-builtin-classes to
@babel/plugin-transform-classes
.Browser and ECMAScript builtins are both wrapped with the helper.
OLD MESSAGE
To make the user-experience as nice as possible, I enabled it by default when extending a class defined in the ES specification. If the user wants to extend another class (for example, a DOM class), it can be declared using the
builtins
option.I hardcoded the list of es classes, but in the future I'd like to ask to the
globals
mantainer if it can be stored in that package (we are already using it inbabel-traverse
).Last thing, I had to disable a test and I don't understand why; I'll search the problem tomorrow because now I'm too sleepy.
cc @WebReflection