-
Notifications
You must be signed in to change notification settings - Fork 2k
Class AST: bound/computed properties, executable body #5208
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
Conversation
@@ -2668,6 +2668,7 @@ exports.Class = class Class extends Base | |||
else if method.bound | |||
@boundMethods.push method | |||
|
|||
return unless o.compiling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the natural representation for executable class body statements is the equivalent AST as if they were just statements in a regular block
So here, we don't have to deal with ExecutableClassBody
/HoistTarget
at all, we just leave the class body Block
as a mix of "class-specific" nodes (ClassMethod
/ClassProperty
/ClassPrototypeProperty
) and other "executable body" nodes, and then the class body AST ends up looking like eg:
# this could correspond to eg:
# class A
# b: ->
# x = 3
# @c: 2
# if y
# z()
type: 'ClassBody'
body: [
{type: 'ClassMethod', ...}
{type: 'ExpressionStatement', ...}
{type: 'ClassProperty', ...}
{type: 'IfStatement', ...}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ones that match class fields should probably output as those Babel node types, I'd think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example of what you mean by "class fields"? See my comment as far as my understanding of how Coffeescript constructs relate to the JS class fields proposal
So basically we are matching Babel for everything that corresponds semantically/structurally to a JS/Babel construct
Eg a static property corresponds to a JS "static class field", so for this:
class A
@b: 2
we generate an AST node with:
type: 'ClassProperty'
static: true
like you'd see in the Babel AST for
class A {
static b = 2
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant the class fields proposal. Though I guess it doesn’t apply here because these are all on the prototype?
class Foo {
a = 1
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I don't think we have a structure that corresponds to non-static JS class fields
I read over #4552 for context. My understanding is that the answer to his first question there ("does the b
property have different semantics?") is "yes", that since b: 3
is set (once) on the prototype, it's not semantically the same as a JS class field eg b = 3
So that's why I introduced ClassPrototypeProperty
AST nodes to represent eg b: 3
, since they're a different construct that doesn't seem to have an equivalent in JS/Babel classes
@GeoffreyBooth this covers the remaining class AST: