Add class fields and private methods

[adrianheine]

Closes #154.

[j-f1]

Does this definition language allow you to specify that the type of computed is false?

[adrianheine]

I would expect so, type is also a literal value. Are you thinking about that?

interface PrivateFieldDefinition {
  key: PrivateName;
  computed: false;
}

[j-f1]

Yep, that’s exactly what I was thinking of.

[adrianheine]

We could introduce a new interface ClassElement with key, value and computed and make MethodDefinition, PublicFieldDefinition and PrivateFieldDefinition extend it.

[danez]

I think that would make sense to create an interface for ClassElement. But I wonder if it makes more sense to not include computed in this new interface and completely leave it out for PrivateFields?

[adrianheine]

How about this:

interface ClassBody <: Node {
    type: "ClassBody";
    body: [ ClassElement ];
}

abstract interface ClassElement <: Node {
    static: boolean;
    computed: boolean;
    key: Expression | PrivateName;
    value: Expression | null;
}

interface MethodDefinition <: ClassElement {
    type: "MethodDefinition";
    value: FunctionExpression;
    kind: "constructor" | "method" | "get" | "set";
}

interface FieldDefinition <: ClassElement {
    type: "FieldDefinition";
}

Some thoughts:

• Whether a ClassElement is private can only be determined by the type of the key (there is no redundancy)
• For private ClassElements, computed is always false (but this fact is not represented)
• For MethodDefinitions with kind "constructor", computed and static are always false (but this fact is not represented)
• Adding static blocks will not be straightforward

[piranna]

Can this be merged? It's blocking support of private methods in babel and babel-eslint...

[adrianheine]

@piranna Does the emitted AST follow this proposal?

[nicolo-ribaudo]

Not in Babel, only babel-eslint follows the ESTree spec.

[piranna]

@piranna Does the emitted AST follow this proposal?

I don't know, I just only use that tools, and found a bug that seems to be directly related to this pull-request to being fixed... :-/

[filmic]

@piranna Babel transpiles code with private methods without issues if you use @babel/plugin-proposal-private-methods.

This PR will eventually solve issue with ESLint failing to traverse code with private methods when using babel-eslint parser.

[piranna]

@piranna Babel transpiles code with private methods without issues if you use @babel/plugin-proposal-private-methods.

I'm already using this plugin without problems... Fair enough, the problem only affect babel-eslint.

This PR will eventually solve issue with ESLint failing to traverse code with private methods when using babel-eslint parser.

Ok, thank you for the clarification 👍

[JounQin]

Any update?

[anthonator]

Anything the community can do to help push this along?

[RReverser]

@adrianheine is this up-to-date with what the tools are doing? If so, feel free to merge, especially since it goes into experimental folder anyway.

[armano2]

Do we have a consensus on how AST structure should look for this, namely private Identifier?

[mysticatea]

I think that private:boolean flag (or visibility:string field) is better than PrivateName node.

In semantics, the AST should represent private fields, private methods, and private member accesses. But private names don't exist. I feel that the current proposal is tied to source code text seriously. It doesn't look AST.

[bradzacher]

This is currently blocking us (typescript-eslint) from adding support for typescript 3.8.

Is there something we can do to help this progress?

[JLHwung]

Per https://github.com/tc39/proposal-static-class-features/, private methods can be static.

[JLHwung]

Recently the support of optional chaining has been added to this proposal, and both ?. PrivateIdentifier and OptionalChain . PrivateIdentifier have been accepted. If we are to be consistent with current ChainingExpression approach in #204, does it mean that we should also add PrivateMemberChain?

interface PrivateMemberChain extends Chain {
  type: "PrivateMemberChain";
  property: PrivateName
}

Personally I lean to merge PrivateMemberChain with MemberChain

interface MemberChain extends Chain {
  type: "MemberChain"
  computed: boolean
  property: Expression
}

with an additional note that if property is a PrivateName, computed must be false.

[JLHwung]

In semantics, the AST should represent private fields, private methods, and private member accesses. But private names don't exist.

Note that in #217 a private names is allowed in the left of a BinaryExpression[operator=in].

[mysticatea]

Note that in #217 a private names is allowed in the left of a BinaryExpression[operator=in].

Thank you for teaching. In that case, I'm fine with the PrivateName node.

[nzakas]

@JLHwung does this proposal reflect what Babel is currently doing? Or should we close in favor of a spec from you?

[JLHwung]

@nzakas Babel splits both FieldDefinition and MethodDefinition into ClassProperty/ClassMethod and their private versions.

Besides that we don't have PrivateMemberExpression but extend the current MemberExpression with PrivateName support.

I can draft a new PR based on Babel's approach. I don't have strong opinion on closing this one since these two are easily interchangeable.

[nzakas]

Cool, thanks. Should we close this PR then?

[RReverser]

@nzakas Given that discussion here sort of depends on decision on optional chaining, I'd say let's leave this open. This PR looks quite reasonable as well, but it's hard to tell which one is better at this point.

[nzakas]

I guess my point is that since this is in the experimental folder, and we're only giving a cursory look at experimental syntax, what's the expectation when two competing specs are submitted?