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

Fix #4464: backticked expressions in class body #4668

Merged

Conversation

GeoffreyBooth
Copy link
Collaborator

Fixes #4464, and allows backticks to provide a workaround for class properties (#4552).

This PR treats backticked expressions in a class body as a special case that doesn’t get hoisted; they just get output in the class body:

class A
  `b: 3`
  c: -> 4
var A;

A = class A {
  b: 3;

  c() {
    return 4;
  }

};

This allows backticks to be used for the not-yet-finalized feature of class properties, as shown above (#4552); or any other unsupported code people might want to put in a class body:

class A
  `get x() { return 42; }`
var A;

A = class A {
  get x() { return 42; };
};

The special case is triggered only when the entire expression is backticked. A line that mixes backticked and unbackticked code, assuming it doesn’t throw a compiler error, will be output as it is now (in the hoisted class body). And the backticked expressions are always output first, above the rest of the class body. @connec @xixixao

@GeoffreyBooth GeoffreyBooth changed the title Fix #4464: backticked expressions in class body [CS2] Fix #4464: backticked expressions in class body Aug 30, 2017
@GeoffreyBooth GeoffreyBooth requested a review from connec August 30, 2017 04:12
@GeoffreyBooth GeoffreyBooth changed the base branch from 2 to master September 8, 2017 04:09
@GeoffreyBooth GeoffreyBooth changed the title [CS2] Fix #4464: backticked expressions in class body Fix #4464: backticked expressions in class body Sep 8, 2017
@connec connec force-pushed the backticked-expressions-in-class-body branch from db5b2fa to f77a98a Compare September 20, 2017 14:06
@connec
Copy link
Collaborator

connec commented Sep 20, 2017

@GeoffreyBooth as I mentioned to you I found a much smaller change that accomplishes the same thing.

Edit: I rolled back most of the other changes since they seem to have been 'side-effects' of that approach.

@connec connec force-pushed the backticked-expressions-in-class-body branch from f77a98a to 657bdad Compare September 20, 2017 14:10
@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Sep 20, 2017

A simpler solution would be great! That's why I wanted you to look at this 😄

Can you push your version on this branch, to update the PR?

@connec
Copy link
Collaborator

connec commented Sep 20, 2017

@GeoffreyBooth
Copy link
Collaborator Author

I don't see your commits.

@connec connec force-pushed the backticked-expressions-in-class-body branch 2 times, most recently from 0a336f2 to 4ec0db4 Compare September 20, 2017 14:58
This uses more of the existing machinery for moving class body
expressions into the initializer.
@connec connec force-pushed the backticked-expressions-in-class-body branch from 4ec0db4 to 1d3af8c Compare September 20, 2017 15:01
@connec
Copy link
Collaborator

connec commented Sep 20, 2017

I had squashed the original one, but I've split it out now.

@@ -1751,11 +1751,10 @@ exports.Class = class Class extends Base
new Block expressions

# Add an expression to the class initializer
#
# NOTE Currently, only methods and static methods are valid in ES class initializers.
# When additional expressions become valid, this method should be updated to handle them.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this comment no longer valid?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wavered on it a little. I felt it was confusing since the check now also allows PassthroughLiterals, and I didn't want to add that to the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I could go the other way and flesh it out a little to indicate that it's the key method for determining whether something appears in the class initializer or executable body?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think more is better. Maybe even explicitly mention class fields/class properties, that when they reach Stage 4 this is where we detect them. That way future PRs to add things like that look more like your version of this PR and less like mine 😄

@connec connec merged commit 921eb3f into jashkenas:master Sep 20, 2017
@connec
Copy link
Collaborator

connec commented Sep 20, 2017

It seems I broke the PR by pushing to origin/master by mistake 😢

I reverted origin/master and reopened #4712

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.

2 participants