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

Merged
merged 10 commits into from Dec 20, 2017

Conversation

@nicolo-ribaudo
Member

nicolo-ribaudo commented Dec 12, 2017

Q                       A
Fixed Issues? Fixes #4480
Patch: Bug Fix? 🎉
Major: Breaking Change?
Minor: New Feature? Yes maybe?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes? Yes. I added globals to plugin-transform-classes, but it should already be in users' node_modules folders because it is used by @babel/traverse
License MIT

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 in babel-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

@hzoo hzoo referenced this pull request Dec 12, 2017

Open

Add this behavior to Babel #11

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Dec 12, 2017

Collaborator

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

Collaborator

babel-bot commented Dec 12, 2017

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

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Dec 12, 2017

Member

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 loose mode?

Member

Andarist commented Dec 12, 2017

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 loose mode?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 12, 2017

Member

We already changed the loose mode transform to be simpler? #4850 unless you mean even more so

Member

hzoo commented Dec 12, 2017

We already changed the loose mode transform to be simpler? #4850 unless you mean even more so

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 12, 2017

Member

@Andarist Some builtins (like HTMLElement, which is used when creating custom elements) don't like being .applyed.

> HTMLElement.apply({}, [])

Uncaught TypeError: Failed to construct 'HTMLElement': Please use the 'new' operator, this DOM object constructor cannot be called as a function.
    at <anonymous>:1:13
Member

nicolo-ribaudo commented Dec 12, 2017

@Andarist Some builtins (like HTMLElement, which is used when creating custom elements) don't like being .applyed.

> HTMLElement.apply({}, [])

Uncaught TypeError: Failed to construct 'HTMLElement': Please use the 'new' operator, this DOM object constructor cannot be called as a function.
    at <anonymous>:1:13
@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 12, 2017

@Andarist many constructors won't work there.

Uint8Array, as well as String, or HTMLElement (and every other from DOM) or CustomEvent and the list go on for mostly every builtin.

WebReflection commented Dec 12, 2017

@Andarist many constructors won't work there.

Uint8Array, as well as String, or HTMLElement (and every other from DOM) or CustomEvent and the list go on for mostly every builtin.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 12, 2017

@nicolo-ribaudo the issue in the test is probably here.

Checking Object.getPrototypeOf(D) === Object fails because the patch/fix needs to put an intermediate constructor in between.

I think that assert.equal(Object.getPrototypeOf(D), Object); should be assert.isTrue(D instanceof Object); or, if you want to be sure about the intermediate constructor, assert.equal(Object.getPrototypeOf(Object.getPrototypeOf(D)), Object);

WebReflection commented Dec 12, 2017

@nicolo-ribaudo the issue in the test is probably here.

Checking Object.getPrototypeOf(D) === Object fails because the patch/fix needs to put an intermediate constructor in between.

I think that assert.equal(Object.getPrototypeOf(D), Object); should be assert.isTrue(D instanceof Object); or, if you want to be sure about the intermediate constructor, assert.equal(Object.getPrototypeOf(Object.getPrototypeOf(D)), Object);

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 12, 2017

@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 WeakMap, just a Map, 'cause there's no way a builtin will suddenly disappear from the engine or it'll be Garbage Collected.

That means that using Map you'll have wider compatibility out of the box and less memory pressure.

WebReflection commented Dec 12, 2017

@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 WeakMap, just a Map, 'cause there's no way a builtin will suddenly disappear from the engine or it'll be Garbage Collected.

That means that using Map you'll have wider compatibility out of the box and less memory pressure.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 12, 2017

Member

Thank you all for the quick feedback, they are really appreciated. I'll update the PR tomorrow.

Member

nicolo-ribaudo commented Dec 12, 2017

Thank you all for the quick feedback, they are really appreciated. I'll update the PR tomorrow.

a.push.apply(a, args);
Constructor = Parent.bind.apply(Parent, a);
return _sPO(new Constructor, Class.prototype);
};

This comment has been minimized.

@Kovensky

Kovensky Dec 14, 2017

Member

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 🤔

@Kovensky

Kovensky Dec 14, 2017

Member

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 🤔

This comment has been minimized.

@WebReflection

WebReflection Dec 14, 2017

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

@WebReflection

WebReflection Dec 14, 2017

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

Show outdated Hide outdated packages/babel-helpers/src/helpers.js
Show outdated Hide outdated packages/babel-plugin-transform-classes/src/index.js
Show outdated Hide outdated packages/babel-plugin-transform-classes/src/vanilla.js
@@ -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__ };

This comment has been minimized.

@Kovensky

Kovensky Dec 14, 2017

Member

This breaks the workaround in #3527 but there's no hope of extending builtins working if the workaround is necessary anyway 🤷‍♀️

@Kovensky

Kovensky Dec 14, 2017

Member

This breaks the workaround in #3527 but there's no hope of extending builtins working if the workaround is necessary anyway 🤷‍♀️

This comment has been minimized.

@WebReflection

WebReflection Dec 14, 2017

this patch is explicitly incompatible with IE <= 10 where you should not extend native builtins

@WebReflection

WebReflection Dec 14, 2017

this patch is explicitly incompatible with IE <= 10 where you should not extend native builtins

This comment has been minimized.

@trusktr

trusktr Jan 31, 2018

But, if we're writing new code that extends builtins...?

@trusktr

trusktr Jan 31, 2018

But, if we're writing new code that extends builtins...?

This comment has been minimized.

@WebReflection

WebReflection Jan 31, 2018

then you are responsible to make such code work as expected otherwise you should stop using JavaScript since even Object could be overwritten 😄

@WebReflection

WebReflection Jan 31, 2018

then you are responsible to make such code work as expected otherwise you should stop using JavaScript since even Object could be overwritten 😄

This comment has been minimized.

@trusktr

trusktr Jan 31, 2018

Gotcha, let me stop using JS on my next web app!

@trusktr

trusktr Jan 31, 2018

Gotcha, let me stop using JS on my next web app!

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 14, 2017

Member

Currently this PR only wraps globals. After thinking a bit about it, I come to the conclusion that classes defined by the builtins option (which would need to be renamed to something like additionalWrap) should also work for non-global variables. That would cover, for example #7022.

Member

nicolo-ribaudo commented Dec 14, 2017

Currently this PR only wraps globals. After thinking a bit about it, I come to the conclusion that classes defined by the builtins option (which would need to be renamed to something like additionalWrap) should also work for non-global variables. That would cover, for example #7022.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 14, 2017

@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?

WebReflection commented Dec 14, 2017

@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?

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 14, 2017

Member

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.

Member

nicolo-ribaudo commented Dec 14, 2017

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.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 14, 2017

remove the list of builtins in this PR

that list won't include HTMLElement though, unless you grab the browser property and filter by comparing typeof global[key] === 'function' && /^[A-Z]/.test(key), right?

WebReflection commented Dec 14, 2017

remove the list of builtins in this PR

that list won't include HTMLElement though, unless you grab the browser property and filter by comparing typeof global[key] === 'function' && /^[A-Z]/.test(key), right?

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 14, 2017

Member

@WebReflection Yes. It could be an option (e.g. builtins: "es" | "browser" | string[]). "browser" can be automatically set by preset-env when targeting a browser.

Member

nicolo-ribaudo commented Dec 14, 2017

@WebReflection Yes. It could be an option (e.g. builtins: "es" | "browser" | string[]). "browser" can be automatically set by preset-env when targeting a browser.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 15, 2017

Member

@WebReflection 46325e1 enables wrapping for DOM classes always, since in non-browsers users won't use them anyway.

Member

nicolo-ribaudo commented Dec 15, 2017

@WebReflection 46325e1 enables wrapping for DOM classes always, since in non-browsers users won't use them anyway.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection Dec 15, 2017

makes, sense as it is now

WebReflection commented Dec 15, 2017

makes, sense as it is now

@xtuc

xtuc approved these changes Dec 19, 2017

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo
Member

hzoo commented Dec 20, 2017

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection commented Dec 20, 2017

LGTM

@justinfagnani

This comment has been minimized.

Show comment
Hide comment
@justinfagnani

justinfagnani Dec 20, 2017

Contributor

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!

Contributor

justinfagnani commented Dec 20, 2017

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!

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 20, 2017

Member

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?

Member

hzoo commented Dec 20, 2017

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?

@hzoo hzoo merged commit 0c885b3 into babel:master Dec 20, 2017

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.23% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 20, 2017

Member

Nice work on following through with this @nicolo-ribaudo and everyone's reviews - good community effort to move this forward finally 😄

Member

hzoo commented Dec 20, 2017

Nice work on following through with this @nicolo-ribaudo and everyone's reviews - good community effort to move this forward finally 😄

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo
Member

nicolo-ribaudo commented Dec 20, 2017

🎉

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:extend-builtins branch Dec 20, 2017

// 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 };

This comment has been minimized.

@jridgewell

jridgewell Dec 20, 2017

Member

Bug! This fallback does not return o.

@jridgewell

jridgewell Dec 20, 2017

Member

Bug! This fallback does not return o.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Dec 20, 2017

Member

Whoops, good catch!

@nicolo-ribaudo

nicolo-ribaudo Dec 20, 2017

Member

Whoops, good catch!

@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr Jan 31, 2018

How do I use this? I'm in Meteor 1.6.1 which uses Babel 7 beta.38.

@nicolo-ribaudo I read in the OP,

If the user wants to extend another class (for example, a DOM class), it can be declared using the builtins option.

What does that mean exactly? Can you provide an example?


I get an error like this:

Uncaught TypeError: Failed to construct 'CustomElement': The result must implement HTMLElement interface

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'))

trusktr commented Jan 31, 2018

How do I use this? I'm in Meteor 1.6.1 which uses Babel 7 beta.38.

@nicolo-ribaudo I read in the OP,

If the user wants to extend another class (for example, a DOM class), it can be declared using the builtins option.

What does that mean exactly? Can you provide an example?


I get an error like this:

Uncaught TypeError: Failed to construct 'CustomElement': The result must implement HTMLElement interface

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'))
@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr Jan 31, 2018

I checked the source,

https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-classes/src/index.js#L8-L14

and I see that HTMLElement is included in globals.browser, so it should just work?

What .babelrc configuration (for a regular Babel build, not Meteor) is needed to make this work?

trusktr commented Jan 31, 2018

I checked the source,

https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-classes/src/index.js#L8-L14

and I see that HTMLElement is included in globals.browser, so it should just work?

What .babelrc configuration (for a regular Babel build, not Meteor) is needed to make this work?

@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr Feb 1, 2018

I made a simple reproduction (using just Babel, no Meteor) showing that I'm using @babel/plugin-transform-classes but I still get the above error:

https://github.com/trusktr/babel/tree/issue-7020

It includes the built output in build/test.js. Open test.html in your browser to see the error.

You can also try npm i && ./node_modules/.bin/babel test.js -o build/test.js to build it.

Did I miss some configuration, or is there a bug?

trusktr commented Feb 1, 2018

I made a simple reproduction (using just Babel, no Meteor) showing that I'm using @babel/plugin-transform-classes but I still get the above error:

https://github.com/trusktr/babel/tree/issue-7020

It includes the built output in build/test.js. Open test.html in your browser to see the error.

You can also try npm i && ./node_modules/.bin/babel test.js -o build/test.js to build it.

Did I miss some configuration, or is there a bug?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.