Use setPrototypeOf for inheritance on any browser with support by ChadKillingsworth · Pull Request #2419 · google/closure-compiler

ChadKillingsworth · 2017-04-09T12:17:31Z

ES6 class static inheritance requires the use of Object.setPrototypeOf. In #2375 I tried multiple ways to work around this because setPrototypeOf caries performance warnings, but all of the things I tried ended up breaking my code in non-trivial ways.

Both Babel and Traceur use setPrototypeOf.

This is becoming a larger issue by the day as more and more code begins to fully take advantage of ES6 style inheritance. I cannot find any real data on the performance hit or if it perhaps it only performs worse when changing a prototype after instantiation.

If this is not acceptable inside Google than we may need to add a flag that opts Google projects out of this, but externally we need to make this change.

Polymer requires this (the code breaks without it).

brad4d · 2017-04-10T18:02:14Z

With things as they are inheritance of static methods in compiled code is not correct by the ES6 spec, but is consistent in old and new browsers.
With this change compiled code behavior will no longer be consistent. Newer browsers will get correct ES6 static method inheritance, while older ones continue with the broken behavior.

I want to do this, but I want to make sure users are warned of this inconsistency appropriately.

I believe we already have checks for stuff like this and treat it as an error in at least some cases.

class A {
  static foo() {}
}

class B extends A {
}

B.foo();

According to
MDN
setPrototypeOf wasn't standardized until ES6, but has been available at least IE11.
Our polyfill for it works if __proto__ is available, which may get a few more browsers,
but still no IE < 11.

@MatrixFrog has some definite opinions on this, so I'm assigning it to him for further discussion.

ChadKillingsworth · 2017-04-11T00:58:28Z

Correct - this only works back through IE11. Babel uses __proto__ as a fallback, but it really doesn't appreciably increase support (you get a few old versions of Opera I believe).

Here's the Polymer code that doesn't work without this:

/**
 * Called by `ElementClass.finalize()`. Ensures this `klass` and
 * *all superclasses* are finalized by traversing the prototype chain
 * and calling `klass.finalize()`.
 * @param {HTMLElement} klass
 * @private
 */
function finalizeClassAndSuper(klass) {
  let proto = klass.prototype;
  let superCtor = Object.getPrototypeOf(proto).constructor;
  if (superCtor.prototype instanceof PolymerElement) {
    superCtor.finalize();
  }
  finalizeClass(klass);
}

It's perfectly valid code.

ChadKillingsworth · 2017-04-11T00:59:00Z

I'd continue to find work arounds except that EVERY other transpiler uses setPrototypeOf.

ChadKillingsworth · 2017-04-11T01:00:31Z

Oh also, as in #2375 if you extend code from another transpiler, the properties won't be enumerable and copying the descriptors fails.

MatrixFrog · 2017-04-11T01:07:48Z

We already polyfill setPrototypeOf using __proto__ so if we just call setPrototypeOf then we're already getting that little bit of extra support. I'm happy to use setPrototypeOf as long as we have some fallback that works on browsers that don't have it (or __proto__) which means we might need to add a flag to control which way the transpilation is done. Which is not great but seems to be necessary as a short term solution until we have ES6 output.

brad4d · 2017-04-11T21:42:43Z

How about this?

Add an argument to the $jscomp.inherits() method to indicate whether it should do setPrototypeOf or copying. The compiler will use this argument to disable the ES6-compatible behavior when the output mode is ES3 and enable it otherwise.

ChadKillingsworth · 2017-04-12T16:25:36Z

Do we gain anything with the flag? setPrototypeOf should provide better support - not less. It doesn't change the semantics of hasOwnProperty. Other than performance this should be a straight up win.

If it's for performance, then sure - this makes sense.

brad4d · 2017-04-12T17:42:00Z

The point of my suggestion is to ensure that folks who are building with ES3 output get consistent behavior across browsers and do not have their code start crashing on older browsers without any warning from us.
Everyone else will benefit from the new setPrototypeOf goodness.
We might also add a warning when compiling such cases to ES3.

ChadKillingsworth · 2017-04-23T22:22:27Z

@brad4d @MatrixFrog I've added a parameter to the inherits function to specify whether the languageOut is ES5 or higher. That was a bit trickier than I expected - quite a few places depend on the number of arguments in the inherits function.

brad4d

Thanks, @ChadKillingsworth
I think this is a step in the right direction.
It'll be great to support the correct ES6 semantics where we can.

@MatrixFrog are you comfortable with this change?
Do you have the bandwidth to shepherd this PR in, or would you rather I did it?

Thanks.

brad4d · 2017-04-26T15:25:24Z

src/com/google/javascript/jscomp/js/es6/util/inherits.js

-  childCtor.prototype = new tempCtor();
-  /** @override */
-    childCtor.prototype.constructor = childCtor;
+$jscomp.inherits = function(childCtor, parentCtor, es5Out) {


Let's be more explicit about what this new argument means.

useSetPrototypeOf

brad4d · 2017-04-26T15:34:43Z

src/com/google/javascript/jscomp/js/es6/util/inherits.js

-  /** @override */
-    childCtor.prototype.constructor = childCtor;
+$jscomp.inherits = function(childCtor, parentCtor, es5Out) {
+  if (es5Out === true && Object.setPrototypeOf) {


I'm inclined to drop the && Object.setPrototypeOf check in favor of this:

if (useSetPrototypeOf) { if ('function' != typeof Object.setPrototypeOf) { throw new Error('Object.setPrototypeOf function is not defined.') }

The useSetPrototypeOf parameter is basically an assertion that Object.setPrototypeOf should be available.
If it isn't, this code should fail with an exception.

We can't do that. Because then IE9 won't work at all. IE9 inheritance still works without setPrototypeOf - just not for static methods.

This is a well known restriction and developers are used to this. See https://babeljs.io/docs/usage/caveats/#internet-explorer

When I suggested the extra argument for $jscomp.inherits() I believed this to be true, which may be false:

All browsers that support all ES5 features also support setPrototypeOf (or __proto__ which allows our polyfill to work) despite the fact that those aren't actually defined in ES5.

FWIW, MDN says that IE11 was the earliest IE version to support them. I don't know right off how to determine whether any IE < 11 should be considered ES5 compliant.

Based on that belief I was expecting we would support these scenarios.

Developer asks for ES3 output, basically saying we cannot count on setPrototypeOf working. In this case we do not try to use setPrototypeOf(), because we want to make sure the code we output has the same behavior on all browsers.

Developer asks for ES5 output, which is effectively a promise not to run our output on a browser that doesn't have full support for ES5, and by my assumption above, for setPrototypeOf(). In this case we use setPrototypeOf() and expect it to be there. If it isn't there, we need to let the developer know by treating that as an error.

Do you know whether my assumption is true or false?
Do you have other problems with this plan?
Thanks.

setPrototypeOf and __proto__ weren't defined until ES2015. Ideally we could target ES5+ output for this, but I know of no way to do this.

IE 9 and 10 are considered ES5 compliant browsers. This is the crux of the difficulty.

Here's the state of things:

ES3 out (IE 8 and lower) -> setPrototypeOf can't be counted on. No ES3 browser supports this.

ES5 out (IE 9-11) -> setPrototypeOf works in IE11, but not in IE 9 or 10. __proto__ only adds in an old set of Opera browsers.

ES6 out -> If only this worked ...

I recognize the desire to protect users, but in this case it's just not possible. Static inheritance doesn't properly work in IE 9 and 10.

@shicks
Hmmm... Sounds like this may be a first use case for environment-specific targeting.

Either we could add a new language_out value for ES5_WITH_SET_PROTOTYPE (or some much better name), or we could add a flag for "make static inheritance work, I know the results won't work right on browsers without setPrototypeOf".

Why do we need this complexity? It only adds more features when it works. It should not break any existing code.

This is a well known restriction to JS devs. The fallback is to copy the descriptors exactly like we do now (which gives most of the support). There are only some edge cases that fail.

I discussed this a bit with @MatrixFrog today.
I think what's needed is a --use_setprototypeof_for_inheritance option to guard this.
If that option isn't given, we pass false to $jscomp.inherits as its useSetPrototypeOf parameter.
$jscomp.inherits throws an error if the argument is true, but Object.setPrototypeOf doesn't exist.

brad4d · 2017-04-27T23:20:49Z

@ChadKillingsworth
Sorry I didn't see your reply.
Here's an scenario to explain why we need this hidden behind a flag.

Project Fooey:

Adopted ES6 classes a year ago and is very happy with them.
Must run on browsers back as far as IE9, but fortunately not earlier, so they specify --language_out=ES5. This allows them to rely on the existence of e.g. Array.prototype.some even though we don't have a polyfill for that. Also they can use getters and setters, which we cannot transpile to ES3.
Is not relying on the "full" ES6 behavior for inheritance of static methods, because of course we don't support it. However, they work with what we have and their code works the same on all browsers.

Now, suppose we fix things so we support ES6 behavior in browsers where we can, but fall back when we can't. No problems at first for Project Fooey (probably), because they wrote their code assuming the old behavior which is still pretty much the same.

Then someone on Project Fooey (or worse on some team that writes a code library Fooey uses), starts writing code that depends on the full ES6 static inheritance working. This is very likely to happen. Not everything gets tested on the older browsers.

Suddenly Project Fooey sees mysterious breakages on the older browsers, and they have no idea why.
Finding the cause could take a very long time and prove very difficult to fix.

ChadKillingsworth · 2017-05-02T01:24:56Z

As discussed, I added a compiler option to control whether setPrototypeOf is used for inheritance. It is on by default. You'll need to add an internal flag to disable it.

ChadKillingsworth · 2017-05-23T13:03:33Z

Rebased and updated to account for the transpilation refactoring by @MatrixFrog.

ChadKillingsworth · 2017-06-05T02:31:37Z

Rebased again

brad4d · 2017-06-05T20:37:06Z

src/com/google/javascript/jscomp/js/es6/util/inherits.js

+    Object.setPrototypeOf(childCtor, parentCtor);
+  } else {
+    /** @constructor */
+    function tempCtor() {}


This declaration generates an error in strict mode compilation.

[synthetic:es6/util/inherits] :61: ERROR - functions can only be declared at top level or immediately within another function in strict mode function tempCtor() {} ^^^^^^^^^^^^^^^^^^^^^^

I will fix it by changing the line to

var tempCtor = function() {};

ChadKillingsworth · 2017-06-06T00:01:50Z

FYI there will be merge conflicts with #2418 on tests. So which ever is merged first the other PR will need updated.

brad4d · 2017-06-12T16:07:04Z

I've imported this and resolved conflicts in the imported version.
It causes one of our internal tests to fail.
I'm looking into that.

brad4d · 2017-06-12T18:08:36Z

Fixed that test & realized more needed to be done to make sure internal default is to have this option off for now. Added a CommandLineRunner flag that defaults to on, because the option itself needs to default to off to make sure our tests run in the expected mode.

Running first line of submission tests now, with the "test all the things" run to happen if that passes.

shicks · 2017-06-13T03:06:31Z

Any reason this can't be done with an @define rather than a whole separate flag? It looks like the only change is in the runtime library.

ChadKillingsworth · 2017-06-13T11:11:59Z

@shicks Is there a way to default an @define inside Google? We want the default externally to be to use setPrototypeOf.

brad4d · 2017-06-13T18:45:14Z

Just had further discussion on this with @concavelenz and @shicks

@ChadKillingsworth I think you'll be happy to hear that the consensus now is we should go with your original intention of just using setPrototypeOf() if possible and doing the property copy loop if not.

The problem described in this comment remains.
#2419 (comment)

However, our justification for allowing this is that teams that do not test on older browsers must assume their code does not work there.

We do need to make an implementation change to this PR, though.
It's problematic to use methods in $jscomp.inherits() that are possibly polyfilled themselves,
so we need to have a $jscomp.setPrototypeOf() method that returns true for success and false for failure.

I've filed issue #2529 for that.

ChadKillingsworth · 2017-06-15T21:43:57Z

I have this work done and compiler tests passing, but I want to test it locally before I push it up (since the runtime tests can't be executed externally).

…Required for proper ES6 class static inheritance.

ChadKillingsworth · 2017-06-18T13:26:08Z

This works on my project - but it needs tested on a project with older browsers (and of course the runtime tests).

brad4d · 2017-06-20T20:00:40Z

imported
looking over the change with @shicks

brad4d · 2017-06-20T22:12:12Z

tests look mostly good so far
just a small change needed to the function type signature of $jscomp.setPrototypeOf. It needs to allow null for the prototype argument.

Also added some suppression comments to eliminate compiler warnings for the 'use ...'; statement strings.

I won't bother pushing these changes back to this PR's branch unless there turns out to be more to do before we submit internally.

brad4d · 2017-06-22T23:26:51Z

Found a few more things to fix, but nothing big.
Continuing internal review.

brad4d · 2017-07-05T16:19:36Z

Working on an internal change to just define $jscomp.setPrototypeOf, then another to do this PR.
I've sent the first of those for review.

brad4d · 2017-07-10T19:01:12Z

Definition of $jscomp.setPrototypeOf is trying to submit now.
I keep hitting type errors with one test or another when different options are enabled, but I think this will do it.

brad4d · 2017-07-11T14:47:58Z

Still trying to submit $jscom.setPrototypeOf change. Delayed due to internal system issue unrelated to the change itself.
Final change is in review, and I hope to submit it immediately afterwards.

Related to #2419 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=161527835

brad4d · 2017-07-13T19:30:31Z

Submit of final change is processing now.

brad4d · 2017-07-17T16:53:57Z

Fix submitted & should be in today's github push.

googlebot added the cla: yes label Apr 9, 2017

brad4d self-assigned this Apr 10, 2017

brad4d assigned MatrixFrog and unassigned brad4d Apr 10, 2017

ChadKillingsworth force-pushed the better-inheritance-round2 branch from 7271ae0 to 7798718 Compare April 23, 2017 22:20

brad4d self-assigned this Apr 26, 2017

brad4d reviewed Apr 26, 2017

View reviewed changes

ChadKillingsworth force-pushed the better-inheritance-round2 branch 2 times, most recently from c0c2be2 to 1946ece Compare May 23, 2017 13:01

ChadKillingsworth mentioned this pull request May 23, 2017

Add transpilation support for properties and computed properties from super #2418

Closed

ChadKillingsworth force-pushed the better-inheritance-round2 branch from 1946ece to 767093d Compare June 5, 2017 02:31

ChadKillingsworth closed this Jun 5, 2017

ChadKillingsworth reopened this Jun 5, 2017

brad4d reviewed Jun 5, 2017

View reviewed changes

brad4d mentioned this pull request Jun 13, 2017

Write $jscomp.setPrototypeOf and rework Reflect.setPrototypeOf and Object.setPrototypeOf to use it #2529

Closed

Use setPrototypeOf for inheritance on any browser which supports it. …

f062eb2

…Required for proper ES6 class static inheritance.

ChadKillingsworth force-pushed the better-inheritance-round2 branch from 767093d to f062eb2 Compare June 18, 2017 13:24

MatrixFrog pushed a commit that referenced this pull request Jul 11, 2017

Implement $jscomp.setPrototypeOf and use it in polyfills.

fa8e6a1

Related to #2419 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=161527835

MatrixFrog removed their assignment Jul 17, 2017

blickly closed this in 1eac969 Jul 18, 2017

teppeis mentioned this pull request Aug 11, 2017

Update Closure Compiler v20170806 compat-table/compat-table#1155

Merged

ChadKillingsworth deleted the better-inheritance-round2 branch November 17, 2020 12:27

Conversation

ChadKillingsworth commented Apr 9, 2017

Uh oh!

brad4d commented Apr 10, 2017

Uh oh!

ChadKillingsworth commented Apr 11, 2017

Uh oh!

ChadKillingsworth commented Apr 11, 2017

Uh oh!

ChadKillingsworth commented Apr 11, 2017

Uh oh!

MatrixFrog commented Apr 11, 2017

Uh oh!

brad4d commented Apr 11, 2017

Uh oh!

ChadKillingsworth commented Apr 12, 2017

Uh oh!

brad4d commented Apr 12, 2017

Uh oh!

ChadKillingsworth commented Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brad4d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad4d commented Apr 27, 2017

Uh oh!

ChadKillingsworth commented May 2, 2017

Uh oh!

ChadKillingsworth commented May 23, 2017

Uh oh!

ChadKillingsworth commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChadKillingsworth commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brad4d commented Jun 12, 2017

Uh oh!

brad4d commented Jun 12, 2017

Uh oh!

shicks commented Jun 13, 2017

Uh oh!

ChadKillingsworth commented Jun 13, 2017

Uh oh!

brad4d commented Jun 13, 2017

Uh oh!

ChadKillingsworth commented Jun 15, 2017

Uh oh!

ChadKillingsworth commented Jun 18, 2017

Uh oh!

brad4d commented Jun 20, 2017

Uh oh!

brad4d commented Jun 20, 2017

Uh oh!

brad4d commented Jun 22, 2017

Uh oh!

brad4d commented Jul 5, 2017

Uh oh!

brad4d commented Jul 10, 2017

Uh oh!

brad4d commented Jul 11, 2017

Uh oh!

ChadKillingsworth commented Apr 23, 2017 •

edited

Loading

ChadKillingsworth commented Jun 6, 2017 •

edited

Loading