Skip to content

Support inheritance of properties which are not enumerable#2375

Closed
ChadKillingsworth wants to merge 2 commits intogoogle:masterfrom
ChadKillingsworth:better-inheritance-transpilation
Closed

Support inheritance of properties which are not enumerable#2375
ChadKillingsworth wants to merge 2 commits intogoogle:masterfrom
ChadKillingsworth:better-inheritance-transpilation

Conversation

@ChadKillingsworth
Copy link
Copy Markdown
Contributor

Fix the ES6 inheritance polyfill to properly inherit properties which are not enumerable. This creates a problem when combining Closure-compiler transpiled code with Babel transpiled code. Babel sets ES6 static class methods as non-enumerable, but they should be inherited.

There are no open source tests so this change makes me somewhat nervous. Is prototype the only property that should be excluded?

cc @shicks

@MatrixFrog
Copy link
Copy Markdown
Contributor

combining Closure-compiler transpiled code with Babel transpiled code

I cannot describe the feeling of doom and dread that comes over me when I read this phrase.

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

@MatrixFrog You might as well get past that.

@MatrixFrog
Copy link
Copy Markdown
Contributor

Why would someone want to use two different transpilers in the same JS program?

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

Because closure-compiler specifically doesn't support some valid ES6 transpilation OR the code is distributed pre-transpiled.

@MatrixFrog
Copy link
Copy Markdown
Contributor

Okay. Well in general I would expect Babel to make rather different choices from us, so I'm surprised it works at all and would generally advise people against depending on it always working.

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

I agree - except Babel is correct here and we aren't :)

@ChadKillingsworth ChadKillingsworth force-pushed the better-inheritance-transpilation branch from e6989db to 09c41c4 Compare March 16, 2017 21:47
@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

So apparently there were some tests. I've fixed those and the change should now be much safer.

@ChadKillingsworth ChadKillingsworth force-pushed the better-inheritance-transpilation branch from 09c41c4 to b574d2f Compare March 16, 2017 21:48
@brad4d brad4d self-assigned this Mar 23, 2017
var propNames = Object.getOwnPropertyNames(parentCtor);
for (var i = 0; i < propNames.length; i++) {
var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]);
if (descriptor && (descriptor.enumerable || (descriptor.writeable && descriptor.configurable))) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misspelled writable here. Update incoming...

@ChadKillingsworth ChadKillingsworth force-pushed the better-inheritance-transpilation branch from b574d2f to c60c268 Compare March 30, 2017 22:53
@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

Updated to correct the spelling of writable.

@ChadKillingsworth ChadKillingsworth force-pushed the better-inheritance-transpilation branch from c60c268 to f960905 Compare March 31, 2017 14:27
var propNames = Object.getOwnPropertyNames(parentCtor);
for (var i = 0; i < propNames.length; i++) {
var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]);
if (descriptor && (descriptor.enumerable || descriptor.configurable))) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe travis is failing, because there's one to many closing parentheses on this line.

@brad4d brad4d force-pushed the better-inheritance-transpilation branch from 45f49da to 5bb9deb Compare April 2, 2017 09:16
@brad4d
Copy link
Copy Markdown
Contributor

brad4d commented Apr 2, 2017

I fixed the extra parenthesis and squashed/rebased the result.
I've imported that and started our standard tests.
Assuming that passes, I'll also need to test all-the-things since this could be a significant behavior change.

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

Sorry about that - I'm juggling too many branches right now.

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

@brad4d This isn't quite right - there are edge cases that just aren't working. I'm trying to find a combination that works properly.

if (Object.defineProperties) {
var propNames = Object.getOwnPropertyNames(parentCtor);
for (var i = 0; i < propNames.length; i++) {
var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use getOwnPropertyDescriptor here, I believe you need to add a require above.

'require es6/object/getownpropertysymbols';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong about this. When I added the 'require' locally to see if it would fix our failing internal test it just added more errors about stuff defined in getownpropertysymbols.js.
Perhaps just adding to the explict externs in the test classes as you've done is the way for me to fix that.

@shicks how do 'require' lines work in these files?
Should they all explicitly require anything they use from other file,
or is that not needed for things declared in externs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brad4d My understanding is the require statements are needed for methods which you can polyfill. However there is no polyfill for getOwnPropertyNames for ES3. So in this case I didn't import them. Object.defineProperties is the same way - no ES3 equivalent.

@brad4d
Copy link
Copy Markdown
Contributor

brad4d commented Apr 7, 2017

@ChadKillingsworth
I had one of our internal-only test fail with a complaint about getOwnProperty names.
I believe it'll be fixed by adding an explicit extern def in that test like you did in one here.

What problems are you referring to when you say this isn't quite right yet?

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

What I had only worked with a single level of inheritance. Multiple levels failed. I think the latest commit fixes it. Also, here's a full test case. Can we add this to the runtime tests? Since they don't run in the open source project it's hard for me to do that.

var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();

function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var Base = function () {
  function Base() {
    _classCallCheck(this, Base);
  }

  _createClass(Base, null, [{
    key: 'origMethod',
    value: function origMethod() {
      return 'hello world';
    }
  }]);

  return Base;
}();

var Intermediate = function (_Base) {
  _inherits(Intermediate, _Base);

  function Intermediate() {
    _classCallCheck(this, Intermediate);

    return _possibleConstructorReturn(this, (Intermediate.__proto__ || Object.getPrototypeOf(Intermediate)).apply(this, arguments));
  }

  _createClass(Intermediate, null, [{
    key: 'intermediateMethod',
    value: function intermediateMethod() {
      return 'intermediate';
    }
  }]);

  return Intermediate;
}(Base);

class MyClass extends Intermediate {}
MyClass.intermediateMethod();
MyClass.origMethod();

@ChadKillingsworth
Copy link
Copy Markdown
Contributor Author

As much as I hate to admit it, I think @MatrixFrog may be right here.

I've come to conclude that the only correct way to handle this would be to use Object.setPrototypeOf - however that has some pretty negative performance issues (but that's exactly what Babel does). Coping the descriptors down then changes hasOwnProperty semantics.

Instead, I'll just spend time fixing any transpilation blockers.

@ChadKillingsworth ChadKillingsworth deleted the better-inheritance-transpilation branch June 6, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants