Add helpers.wrapCtor to fix T7328

[jdalton]

This PR adds helpers.wrapCtor to fix T7328.

edit:
Fixes #4269

[loganfsmyth]

A couple concerns on this:

• This introduces a call to Object.setPrototypeOf at instantiation time, which will likely be a performance issue for classes that are instantiated often, as far as I know?
• We don't have a great migration path at the moment for new helpers, because users may be using helpers loaded via babel-runtime or babel-plugin-external-helpers, and if their versions happen to be out of sync with the version of babel-plugin-transform-es2015-classes, this would cause their code to start throwing due to the missing helper.

At the moment, we have https://github.com/loganfsmyth/babel-plugin-transform-builtin-extend which allows for extending real ES6 classes by using Reflect.construct which should be available pretty much anywhere that you want to use a real ES6 class. Would that be sufficient for your case?

[jdalton]

@loganfsmyth

This introduces a call to Object.setPrototypeOf at instantiation time, which will likely be a performance issue for classes that are instantiated often, as far as I know?

I can tweak the implementation.

At the moment, we have https://github.com/loganfsmyth/babel-plugin-transform-builtin-extend which allows for extending real ES6 classes by using Reflect.construct which should be available pretty much anywhere that you want to use a real ES6 class. Would that be sufficient for your case?

I don't believe that a doable solution. Node 4 has built-ins like Map but doesn't have have Reflect.construct or native class support. Babel desugars the class code to plain JS which then runs into issues with built-ins.

[codecov-io]

Current coverage is 87.71%

Merging #3582 into master will increase coverage by 0.22%

@@             master      #3582   diff @@
==========================================
  Files           194        194          
  Lines          8949       9464   +515   
  Methods        1007       1070    +63   
  Messages          0          0          
  Branches       2004       2157   +153   
==========================================
+ Hits           7829       8301   +472   
- Misses         1120       1163    +43   
  Partials          0          0          

Powered by Codecov. Last updated by 40ec299...f5b1a39

[loganfsmyth]

I don't believe that a doable solution. I support Node 0.12 and up which has built-ins like Map but doesn't have have Reflect.construct or native class support. Babel desugars the class code to plain JS which then runs into issues with built-ins.

Would you be up for me taking your example code and including it as a third option for https://github.com/loganfsmyth/babel-plugin-transform-builtin-extend/blob/master/src/index.js? I do think we can explore this more, but it's definitely not clear that there will be an easy way to land this as part of Babel 6 core.

[jdalton]

Is the issue that it's currently in babel-helpers?

Should the fix be moved to another part of the code base?
(Maybe another babel-helper-xyz package?)

[loganfsmyth]

The two points from before are my core concerns. I don't know how to do this in the general case without .setPrototypeOf and that's a performance hit + not supported well on IE. Adding a new helper would likely require a major version bump with the way things are set up right now. Handling the primary case of extending other Babel classes via Babel core and extending builtin types via extra transforms seems like the best way forward at the moment.

[jdalton]

1. There is existing setPrototypeOf use in babel-helpers, look to helpers.inherits.
   
   This addition mimics the establish usage. BTW helpers.inherits is used for every class that extends another which is the same scenario for this addition.
2. I've continued tweaking the PR to avoid setPrototypeOf for common cases

So with that are there any other concerns?

Would moving the fix to a separate babel-helper-xyz package,
or even including in babel-plugin-transform-es2015-classes, avoid your versioning concern?

[loganfsmyth]

1. The existing usage only calls it once at class declaration time, whereas calling it on construction will dramatically increate class instantiation time.
2. True enough. Having a whitelist of arbitrary letters seems like a bit of a ugly approach though.

To try to elaborate more on this, the very fact that this is such a hard thing to do (and do well), to me at least, indicates that it's a better fit for an opt-in feature implemented as a plugin. Babel has always had this as one of the caveats called on in our docs: https://babeljs.io/docs/usage/caveats/#classes, and we provide the extra plugin for the cases where users do want to do this.

As can be seen from your proposed implementation, getting this right is non-trivial. It requires non-standard functionality that not all ES5 environments support, combined with what amounts to a whitelist of class names to keep the performance acceptable.

What's your motivation for pushing back so strongly against the existing plugin approach? I agree it's not perfect, but it seems much more desirable for Babel core to be consistent.

[jdalton]

True enough. Having a whitelist of arbitrary letters seems like a bit of a ugly approach though.

It can be as simple/detailed or clean/ugly as you all would like. I'm just kicking stuff around to see what meets an acceptable bar.

To try to elaborate more on this, the very fact that this is such a hard thing to do (and do well), to me at least, indicates that it's a better fit for an opt-in feature implemented as a plugin.

It's not hard to do it right-ish.

As can be seen from your proposed implementation, getting this right is non-trivial. It requires non-standard functionality that not all ES5 environments support, combined with what amounts to a whitelist of class names to keep the performance acceptable.

It's using standard lang features. The same features that are already being used for class inheritance wiring. Listing of a handful spec'ed constructors is just one way to manage it.

What's your motivation for pushing back so strongly against the existing plugin approach? I agree it's not perfect, but it seems much more desirable for Babel core to be consistent.

It's core functionality that's simple enough to tackle in a few lines (~220bytes) vs. a significantly more complex and heavy plugin is all.

Is there a perf goal you're trying to stay within or a benchmark to reference?

[loganfsmyth]

It's core functionality that's simple enough to tackle in a few lines (~220bytes) vs. a significantly more complex and heavy plugin is all.

I admit I don't understand what's heavy about the plugin specifically. In your wrapper case, if someone isn't using babel-runtime or external helpers this will add once more chunk of code to every file with a class declaration, even if they don't extend natives, where the plugin case will only apply to the specific files that extend the builtin. Byte-size wise, mine is larger, but only because I've favored readability over size. I'd bet once gzipped and/or minified, the size in either case would essentially be negligible.

Is there a perf goal you're trying to stay within or a benchmark to reference?

Perf-wise, my main concern is avoiding the setPrototypeOf in frequently-instantiated classes since it's slow. Using the early bail-out approach you've got now is probably as close as we'll get if we want to go this route.

I definitely have concerned about rollout. If we wanted to get this into Babel 6 without separate plugin, I think it would have to be an opt-in feature via an option on the transform, since we don't have a great way to incorporate new helpers into things without causing regressions in existing installs. It seems doable with an opt-in extendableBuiltins: true flag to the class transform that would only apply your new logic when enabled. Perhaps aim to use this as an opportunity to validate the approach before integrating it more in a future version?

You know more about this than I do, but would an approach more along the lines of

    if (ctor.toString() === '[native code]' || ctor.toString().startsWith('class'))

be shorter and more consistent? Part of my concern with the whitelist and args checks is that if we aim to support this, it should be for all constructors lacking [[Call]], including real ES6 classes used alongside transpiled libraries.

I also definitely want feedback from others on this.

[jdalton]

I admit I don't understand what's heavy about the plugin specifically.

It requires shimming Reflect and friends for all but Node 6.
Which means this:

class MapCache extends Map {
  clear() {
    super.clear();
    return this;
  }
};

is transpiled into 9kB (min+gzip) of support code using transform-runtime.

this will add once more chunk of code to every file with a class declaration, even if they don't extend natives, where the plugin case will only apply to the specific files that extend the builtin.

This will only add the helper if they use extends, not to every class declaration, and shared by all other definitions in a given file.

Perf-wise, my main concern is avoiding the setPrototypeOf in frequently-instantiated classes since it's slow. Using the early bail-out approach you've got now is probably as close as we'll get if we want to go this route.

Cool!

I definitely have concerned about rollout. If we wanted to get this into Babel 6 without separate plugin, I think it would have to be an opt-in feature via an option on the transform, since we don't have a great way to incorporate new helpers into things without causing regressions in existing installs. It seems doable with an opt-in extendableBuiltins: true flag to the class transform that would only apply your new logic when enabled. Perhaps aim to use this as an opportunity to validate the approach before integrating it more in a future version?

Sounds like a great start!

You know more about this than I do, but would an approach more along the lines of

    if (ctor.toString() === '[native code]' || ctor.toString().startsWith('class'))

I played around with native code checks in one of the implementation passes. We could post a few implementations and then throw things like large constructors at them to see how well they handle.

Part of my concern with the whitelist and args checks is that if we aim to support this, it should be for all constructors lacking [[Call]], including real ES6 classes used alongside transpiled libraries.

We could modify the _classCallCheck to support that. I'll kick it around.

[searls]

Did a solution to this issue ever get merged or is it still outstanding?

Is the following statement true: "it is currently not possible to instantiate a native ES class which extends a babel-transpiled ES class under Node.js v6"? Based on all my testing, the answer is "yes", which was pretty alarming to realize.

[orneryd]

This is still an issue for me. Is this going to be merged in soon?

[hzoo]

Going to close in favor of #4480 (comment)

[jdalton]

Ah neat! @WebReflection Thank you!