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

Class prototype property AST #5205

Merged
merged 3 commits into from
Apr 28, 2019

Conversation

helixbass
Copy link
Collaborator

@GeoffreyBooth PR for class "prototype property" AST

As far as I'm aware, there's no corresponding proposed JS syntax/Babel AST for class properties that get attached to the prototype eg:

  class A
    b: 3

So this PR introduces a new node class/AST type ClassPrototypeProperty to represent these

Also contains a fix for #5204 (the invalid JS for computed class prototype properties, not my second comment in that issue). If you'd prefer I could extract the fix to a separate PR against master

src/nodes.coffee Outdated
@@ -2883,7 +2896,11 @@ exports.ExecutableClassBody = class ExecutableClassBody extends Base
# The class scope is not available yet, so return the assignment to update later
assign = @externalCtor = new Assign new Value, value
else if not assign.variable.this
name = new (if base.shouldCache() then Index else Access) base
name =
if base instanceof ComputedPropertyName
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the fix for #5204

src/nodes.coffee Outdated
if base instanceof ComputedPropertyName
new Index base.value
else
new (if base.shouldCache() then Index else Access) base
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you remove the conditional logic here (so just new Access base) no tests break. But I don't feel completely confident removing it. I traced the code's origin back to #4354


isStatement: YES

astProperties: (o) ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This mimics the properties of Babel AST types like ClassProperty/ObjectProperty

@GeoffreyBooth
Copy link
Collaborator

This looks fine. One thing to consider is class fields: https://github.com/tc39/proposal-class-fields

Which are currently in Stage 3 but are already shipped in Chrome and Node 12, so we’ll probably need to support them soon.

@helixbass
Copy link
Collaborator Author

Extracted the fix for #5204 to a PR against master (#5206). So once that gets merged, if you merge master into ast, I can add AST tests here for static computed class properties as well

One thing to consider is class fields

Ok so my understanding is this:

"Class fields" in JS eg

class A {
  x = 3
}

represent properties set on new class instances, so basically correspond to the Coffeescript:

class A
  constructor: ->
    @x = 3

I personally find the JS syntax weird, I guess I'm more accustomed to Python/Ruby/Coffeescript-style initialization of instances inside the constructor

So I'd assume we don't need to provide a syntax that compiles to JS class fields (since you can achieve the equivalent by initializing inside the constructor)?

Then I'm also familiar with arrow function "class fields" (I don't know the actual JS nomenclature/proposals) eg

class A {
  b = () => 3
}

And this seems to correspond to Coffeescript bound methods eg

class A
  b: => 3

But then Coffeescript's "prototype properties" eg

class A
  b: 3

are a different thing that just get set once per class (on the prototype) rather than once per instance. If there's a JS proposal for this I haven't seen it, which is why I create a new (non-Babel) AST type ClassPrototypeProperty in this PR to represent them

@GeoffreyBooth GeoffreyBooth merged commit 7b2fb18 into jashkenas:ast Apr 28, 2019
@helixbass helixbass deleted the class-prototype-property-ast branch April 29, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants