add class features #222

JLHwung · 2020-05-28T21:16:35Z

View Rendered Text

This PR adds support to three stage-3 class features proposals. It shows Babel's current approach except for the following node type naming differences:

Babel	This PR
ClassProperty	PropertyDefinition
ClassPrivateProperty	PrivatePropertyDefinition
ClassPrivateMethod	PrivateMethodDefinition

It is named in the *Definition pattern, aligning to the MethodDefinition defined in es2015.

This PR also offers an alternative design of #180. In #180 PrivateMemberExpression has been introduced for MemberExpression[property=PrivateName] in this PR, which will have impact on #204 given that this?.#priv is now supported.

As the class proposals have been out there for quite some time and they are widely adopted in React/TypeScript communities, especially the class fields proposal, I will expect some tools are depending on Babel class features AST. Building consensus based on this approach can minimize the friction supporting new ESTree releases.

Fixes #154
Closes #180

--- Edits ---
Updated according to review comments. PTAL.

nzakas

Seems reasonable at this phase.

mysticatea · 2020-06-04T20:38:40Z

I wonder if the Private* prefix is needed or not. I love consistency.

MemberExpression handles both public and private.
There are four kinds of namespaces: public keys, static public keys, private keys, and static private keys. MethodDefinition doesn't have Static* prefix for static keys. (also I'm afraid of the combination explosion of such prefixes.)

michaelficarra · 2020-06-04T20:51:26Z

experimental/class-features.md

+
+A private name refers to private class elements. For a private name `#a`, its `id.name` is `a`.
+
+When the property of a member expression is a private name, its `computed` is always `false`.


This seems pretty bad. Why not just have a PrivateMemberExpression? The reason to unify them would be that we expect people to consider them similar for some purpose. I don't expect too much of that.

Some reasons that I can tell

Babel has OptionalMemberExpression which corresponds to OptionalChain

PrivateOptionalMemberExpression for obj?.#x can be confusing

There is no PrivateMemberExpression like production in the spec

experimental/class-features.md

bradzacher · 2020-06-05T06:53:08Z

I understand the want to align with MethodDefinition, but I question if it's a good idea to rename ClassProperty to PropertyDefinition. The ClassProperty name has existed for quite a long time, and as such it has been used for a long time. It is very, very prolific throughout the ecosystem.

Unlike the optional chaining changes, which apply to a feature that has only recently been implemented in most of the ecosystem, renaming class properties will be a pretty significant breaking change and will likely be a pretty painful change for users to navigate.

mysticatea · 2020-06-05T08:22:23Z

experimental/class-features.md

+    value: Expression;
+    computed: boolean;
+    static: boolean;
+    private: boolean;


I love consistency. If we add private:boolean to (Method|Property)Definition, I'd like to add it to MemberExpression as well. Or remove it from both.

I think it's best to remove from both; otherwise we're duplicating same information which explicitly goes against one of the design goeals ("Unique") in the Philosophy.

I agree on @RReverser that we don't need .private if we can check .key.type === "PrivateIdentifier". While I admit that .private is handier, the ergonomic trade-off here is still acceptable.

Should another private-like key (PrivateComputedExpression?) be supported in the future, we can introduce .private at that time.

JLHwung · 2020-06-05T16:11:48Z

@bradzacher The ClassProperty is used in Babel AST. Given that PropertyDefinition now deviates a lot from Babel AST design. I would rather stay on the current names instead of silently breaking tools. (i.e. private: boolean is added)

experimental/class-features.md

Co-authored-by: Ingvar Stepanyan <me@rreverser.com>

experimental/class-features.md

JLHwung · 2020-06-17T18:24:47Z

@RReverser friendly ping, does the current AST design look good to you?

RReverser · 2020-06-17T18:44:57Z

Oh yeah, sorry, I haven't noticed that private: boolean has been removed since my last review; LGTM, especially for experimental :)

bradzacher · 2020-06-17T22:23:40Z

Given that PropertyDefinition now deviates a lot from Babel AST design

Sorry, could you clarify how? I don't see how this deviates from the babel AST (or the AST emitted from babel-eslint).

It looks like the current ClassProperty spec introduced by babel and used in the ESLint ecosystem for many years matches your proposed spec exactly (with the only variance being the private property piece)

AST explorer example:
https://astexplorer.net/#/gist/592095bc625a5243ab44009fd1d62863/15d212b3d4439339de5fd36cb0b057d293d0d003

TS-ESTree spec (which was built to match the babel implementation):
https://github.com/typescript-eslint/typescript-eslint/blob/4c9293bd5ecfb475bfd4f2240fb6e55186ff3e55/packages/typescript-estree/src/ts-estree/ts-estree.ts#L583-L606

The ClassProperty name has been around and in use for many, many years, so this will cause a lot of pain for tool owners and users to migrate to.

If that's what the direction going forward is - that's fine. I just want to ensure it's a conscious decision to introduce the breakage.

JLHwung · 2020-06-18T14:26:08Z

The differences between current approach and Babel's approach can be viewed here

Notable changes are:

ClassPrivateProperty and ClassProperty is merged, likewise ClassPrivateMethod
Naming differences
PrivateName has been replaced by PrivateIdentifier

On the AST policy of babel and estree, we haven't stated formally on our website but I believe what @nicolo-ribaudo proposed here is close to the team consensus, that estree does not include experimental features and Babel's proposal AST design should be considered experimental, too. That said, we as Babel team don't intend to bring more fragments to the ECMAScript AST, that's why we have proposed an update of our process to work with estree since stage-1.

Babel introduced ClassPrivateProperty in babel/babylon#260 and back that time there have been discussions about naming conventions. The decision at that time was to merge ClassPrivateProperty and ClassProperty when the class proposals reached stage-3. Now that they are stage-3 and we may consider merging ClassPrivateProperty and ClassProperty.

The ClassProperty name has been around and in use for many, many years, so this will cause a lot of pain for tool owners and users to migrate to.

Yeah I understand the frictions here, but keep in mind that class proposals are still stage-3, which means they are still experimental, and by nature, open to breaking changes. It's a short time window that we still have opportunities to value long term AST consistency/teachability over short term community frictions.

JLHwung · 2020-07-03T21:08:31Z

@nzakas I think we have reached consensus and it can merged as-is.

nzakas · 2020-07-07T19:50:07Z

Sounds good!

piranna · 2020-07-07T20:03:04Z

In what Babel version will this be available?

JLHwung · 2020-07-07T21:00:32Z

@piranna Babel has different AST shape with ESTree: https://github.com/babel/babel/blob/main/packages/babel-parser/ast/spec.md#classproperty

All stage-3 class features are supported on Babel 7.10.

piranna · 2020-07-08T06:54:20Z

@piranna Babel has different AST shape with ESTree: babel/babel:packages/babel-parser/ast/spec.md@main#classproperty

All stage-3 class features are supported on Babel 7.10.

Sorry, I wanted to say eslint.

add class features

5c442f1

nzakas approved these changes Jun 1, 2020

View reviewed changes

michaelficarra reviewed Jun 4, 2020

View reviewed changes

Merge PrivateDefinition and PublicDefinition

886847c

JLHwung requested a review from nzakas June 5, 2020 03:15

JLHwung commented Jun 5, 2020

View reviewed changes

experimental/class-features.md Outdated Show resolved Hide resolved

mysticatea reviewed Jun 5, 2020

View reviewed changes

experimental/class-features.md Outdated Show resolved Hide resolved

mysticatea reviewed Jun 5, 2020

View reviewed changes

experimental/class-features.md Outdated Show resolved Hide resolved

mysticatea reviewed Jun 5, 2020

View reviewed changes

JLHwung added 3 commits June 5, 2020 10:52

add private: boolean to MemberExpression

ec27102

initializer is optional

de6148e

update PrivateName interface

0125693

refine text

b1ba831

JLHwung force-pushed the add-class-features branch from 2cabb33 to b1ba831 Compare June 5, 2020 16:42

RReverser reviewed Jun 5, 2020

View reviewed changes

experimental/class-features.md Outdated Show resolved Hide resolved

JLHwung and others added 2 commits June 5, 2020 15:02

Update experimental/class-features.md

083f826

Co-authored-by: Ingvar Stepanyan <me@rreverser.com>

address review comments

19f342e

This comment has been minimized.

Sign in to view

mysticatea reviewed Jun 6, 2020

View reviewed changes

experimental/class-features.md Outdated Show resolved Hide resolved

JLHwung added 2 commits June 8, 2020 09:42

PrivateName => PrivateIdentifier

04a2841

remove private property with respect to the Unique philosophy

8759fc8

JLHwung force-pushed the add-class-features branch from 8b9aa92 to 8759fc8 Compare June 8, 2020 14:07

nzakas approved these changes Jun 8, 2020

View reviewed changes

mysticatea approved these changes Jun 9, 2020

View reviewed changes

RReverser approved these changes Jun 17, 2020

View reviewed changes

armano2 mentioned this pull request Jun 29, 2020

TypeScript 3.8 Syntax Support typescript-eslint/typescript-eslint#1436

Closed

nzakas merged commit 2218fde into estree:master Jul 7, 2020

JLHwung deleted the add-class-features branch July 7, 2020 20:44

3cp mentioned this pull request Oct 28, 2020

FieldDefinition -> ClassProperty? meriyah/meriyah#134

Closed

tchetwin mentioned this pull request Jan 25, 2021

Update AST node names to match ESTree [breaking change] acornjs/acorn-private-class-elements#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add class features #222

add class features #222

JLHwung commented May 28, 2020 •

edited

nzakas left a comment

mysticatea commented Jun 4, 2020

michaelficarra Jun 4, 2020

JLHwung Jun 4, 2020

bradzacher commented Jun 5, 2020

mysticatea Jun 5, 2020

RReverser Jun 5, 2020

JLHwung Jun 8, 2020

JLHwung commented Jun 5, 2020

This comment has been minimized.

JLHwung commented Jun 17, 2020

RReverser commented Jun 17, 2020

bradzacher commented Jun 17, 2020

JLHwung commented Jun 18, 2020

JLHwung commented Jul 3, 2020

nzakas commented Jul 7, 2020

piranna commented Jul 7, 2020

JLHwung commented Jul 7, 2020

piranna commented Jul 8, 2020


		A private name refers to private class elements. For a private name `#a`, its `id.name` is `a`.

		When the property of a member expression is a private name, its `computed` is always `false`.

add class features #222

add class features #222

Conversation

JLHwung commented May 28, 2020 • edited

View Rendered Text

nzakas left a comment

Choose a reason for hiding this comment

mysticatea commented Jun 4, 2020

michaelficarra Jun 4, 2020

Choose a reason for hiding this comment

JLHwung Jun 4, 2020

Choose a reason for hiding this comment

bradzacher commented Jun 5, 2020

mysticatea Jun 5, 2020

Choose a reason for hiding this comment

RReverser Jun 5, 2020

Choose a reason for hiding this comment

JLHwung Jun 8, 2020

Choose a reason for hiding this comment

JLHwung commented Jun 5, 2020

This comment has been minimized.

JLHwung commented Jun 17, 2020

RReverser commented Jun 17, 2020

bradzacher commented Jun 17, 2020

JLHwung commented Jun 18, 2020

JLHwung commented Jul 3, 2020

nzakas commented Jul 7, 2020

piranna commented Jul 7, 2020

JLHwung commented Jul 7, 2020

piranna commented Jul 8, 2020

JLHwung commented May 28, 2020 •

edited