Skip to content
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 transpilation support for properties and computed properties from super #2418

Conversation

ChadKillingsworth
Copy link
Collaborator

This adds support for property references off of super which has been missing from our transpilation support.

@brad4d brad4d self-assigned this Apr 10, 2017
}
});

if (exprRoot.getParent().isNew()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add code examples to make it clearer what each of these cases represent.

e.g.
if (exprRoot.getParent().isNew()) {
// new super()

case MEMBER_FUNCTION_DEF:
case GETTER_DEF:
case SETTER_DEF:
case COMPUTED_PROP:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add COMPUTED_PROP? Are you trying to support something like this?

class Foo extends Bar {
  [super.something]() {}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No - need to recognize access to super within a computed property function:

class Foo extends Bar {
  [someThing]() {
    super.otherThing();
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

potentialCallee.replaceChild(node, superName.cloneTree());
} else if (enclosingMemberDef.isStaticMember()) {
if (callTarget.isSuper()) {
compiler.report(JSError.make(node, INVALID_SUPER));
Copy link
Contributor

Choose a reason for hiding this comment

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

Before doing this PR, I think we'd better have one that removes support for calling super() in a non-constructor method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Submitted as #2452

for (;
exprRoot.getParent().isGetElem() || exprRoot.getParent().isGetProp();
exprRoot = exprRoot.getParent()) {
if (exprRoot.getParent().getFirstChild() != exprRoot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going too far.
If you have super.foo.bar.baz(), we only need to go as far as super.foo to determine what to do.

@ChadKillingsworth
Copy link
Collaborator Author

Currently waiting on #2419 and #2452

@brad4d
Copy link
Contributor

brad4d commented Jun 1, 2017

This should be unblocked now that #2452 is closed.
@ChadKillingsworth I'll take another look once you've resolved conflicts and addressed the remaining review comments.

@ChadKillingsworth
Copy link
Collaborator Author

@brad4d ready for review again.

@brad4d
Copy link
Contributor

brad4d commented Jun 5, 2017

The Travis failure was probably because our build was broken over the weekend.
It's fixed now, but I guess it doesn't automatically rerun.

@ChadKillingsworth
Copy link
Collaborator Author

You can trigger travis to rerun by closing then re-opening a PR.

@brad4d
Copy link
Contributor

brad4d commented Jun 5, 2017

importing and testing...

@brad4d
Copy link
Contributor

brad4d commented Jun 5, 2017

primary testing looks good
scheduling a 'test all the things' run now

@brad4d
Copy link
Contributor

brad4d commented Jun 6, 2017

Tests looking good.
Sent to @MatrixFrog for internal review.

@brad4d
Copy link
Contributor

brad4d commented Jun 7, 2017

Adding tests for access to a property defined with a getter and a setter before submitting.

@brad4d
Copy link
Contributor

brad4d commented Jun 8, 2017

@ChadKillingsworth
I've added a couple of tests and pushed them back to you.
The runtime test shows that things aren't quite right yet.
The unit test I believe shows why.
The super transpilation here is causing the super setter to modify the prototype object.

@brad4d
Copy link
Contributor

brad4d commented Jun 9, 2017

@ChadKillingsworth
Transpiling super.p = strikes me as tricky to get right and rarely useful to do anyway.
Perhaps you'd prefer to just add a CANNOT_CONVERT_YET error message for that case?

@MatrixFrog
Copy link
Contributor

I would lean toward CANNOT_CONVERT rather than CANNOT_CONVERT_YET but I may be too much of a pessimist

@ChadKillingsworth
Copy link
Collaborator Author

I've got code that depends on transpiling super properties - and polymer uses it. It's not that hard to fix, but it'll probably need a runtime helper injected.

I'll work on it this week.

@ChadKillingsworth
Copy link
Collaborator Author

ChadKillingsworth commented Jun 13, 2017

Disallowed assigning to a super property. @brad4d I assume you'll need to fix the runtime test. Since those don't run externally, I would just be guessing.

For posterity, here's how Babel transpiles assigning to a super property:

var _set = function set(object, property, value, receiver) {
  var desc = Object.getOwnPropertyDescriptor(object, property);
  if (desc === undefined) {
    var parent = Object.getPrototypeOf(object);
    if (parent !== null) {
      set(parent, property, value, receiver);
    }
  } else if ("value" in desc && desc.writable) {
    desc.value = value;
  } else {
    var setter = desc.set;
    if (setter !== undefined) {
      setter.call(receiver, value);
    }
  }
  return value;
};

Foobar.prototype.someMethod() {
  _set(Foobar.prototype.__proto__ || Object.getPrototypeOf(Foobar.prototype),
      "someprop", "someval", this);
}

@brad4d
Copy link
Contributor

brad4d commented Jun 13, 2017

Imported.
Working on runtime_test and running tests.

@brad4d
Copy link
Contributor

brad4d commented Jun 14, 2017

submitted internally
should be in the next push to github

Yannic pushed a commit to Yannic/com_google_closure_compiler that referenced this pull request Jul 6, 2017
…roperties

Closes google#2418

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=158983919
@ChadKillingsworth ChadKillingsworth deleted the transpile-super-properties branch June 6, 2020 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants