Private Class Methods Stage 3: Private Accessors by tim-mc · Pull Request #9101 · babel/babel

tim-mc · 2018-11-29T19:25:25Z

Q	A
Fixed Issues?	babel/proposals#22
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Add private accessors support
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?	No
License	MIT
Sponsor	@bloomberg

This is a follow-up to #8654. It adds support for private accessors within the current babel-plugin-class-features + babel-plugin-proposal-private-methods plugin combination.

cc: @robpalme @littledan @syg @jridgewell @nicolo-ribaudo

babel-bot · 2018-11-29T19:30:22Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9811/

tim-mc · 2018-11-29T19:43:16Z

I'd like to get some feedback on how this should be handling the usage of update expressions and certain assignment expressions. I'm not sure what exactly the expectation is for how to handle these situations:

// update expressions
this.#privateSetter++;
this.#privateSetter--;
++this.#privateSetter;
// etc.

// assignment expressions
this.#privateSetter += 'foo';
this.#privateSetter -= 'foo';

I made a method that checks for these scenarios which is used in both the spec and loose private name handlers.

My main question is: should these usages be throwing errors? If so,

Should they be code frame errors?
Is this the proper place to throw an error? I considered putting it over here in features.js, but since this isn't really a check for a "feature," that seemed wrong.

If they should not be throwing, what would be the proper way to handle these?

...babel-plugin-proposal-private-methods/test/fixtures/private-method-loose/accessors/output.js

nicolo-ribaudo · 2018-11-29T23:04:50Z

Can't ++this.#privateSetter; be desugared to this.#privateSetter = this.#privateSetter + 1, which should set it to NaN?

++this.#privateGetter; should instead throw at runtime, since it is trying to set a non-writable property.

Btw, I think that these cases are easier if you rely on the existing fields descriptor handling logic.

tim-mc · 2018-12-24T19:34:20Z

@nicolo-ribaudo I've made the updates to the PR that we discussed.

packages/babel-helper-create-class-features-plugin/src/index.js

nicolo-ribaudo · 2018-12-27T23:00:04Z

packages/babel-helpers/src/helpers.js

+      }
+
+      descriptor.value = value;
+      return value;


return value; should be outside the if/else, because we always need to return it.

nicolo-ribaudo · 2018-12-27T23:02:06Z

packages/babel-helper-create-class-features-plugin/src/fields.js

+            methodId:
+              isMethod && isInstance && prop.node.kind === "method"
+                ? prop.scope.generateUidIdentifier(name)
+                : undefined,


Nit: could you move the methodId definition inside an if condition, like you did for getId and setId?

nicolo-ribaudo · 2018-12-27T23:04:13Z

packages/babel-helper-create-class-features-plugin/src/fields.js

  return {
    staticNodes,
-    instanceNodes,
+    instanceNodes: instanceNodes.filter(instanceNode => instanceNode),


Nit: we usually use .filter(Boolean)

packages/babel-helper-create-class-features-plugin/src/index.js

nicolo-ribaudo · 2018-12-27T23:07:04Z

This PR looks almost ready!

tim-mc · 2018-12-27T23:16:46Z

@nicolo-ribaudo requested changes have been made.

nicolo-ribaudo · 2018-12-27T23:30:07Z

Can you add a test for the helper change? Something like

let foo;
class Cl {
  set #foo(v) { return 1 }
  test() {
    foo = this.#foo = 2;
  }
}

new Cl().test();

expect(foo).toBe(2);

Also, it would be good to have the following tests:

duplicated-names/get-set // does not throw
duplicated-names/set-get // does not throw
duplicated-names/get-get // throws at compile time
duplicated-names/set-set // throws at compile time
duplicated-names/get-method // throws at compile time
duplicated-names/method-get // throws at compile time
duplicated-names/set-method // throws at compile time
duplicated-names/method-set // throws at compile time
duplicated-names/method-method // throws at compile time. Not really related to this PR, but since we are adding all the other tests it would be good.
accessors/set-only-getter // throws at runtime (e.g. get #foo() {} ... this.#foo = 2)
accessors/get-only-setter // returns undefined
accessors-loose/set-only-getter // throws at runtime
accessors-loose/get-only-setter // returns undefined
Move the testUpdates functions to accessors/updates and accessors-loose/updates
Move the remaining code from private-methods/accessors and private-methods-loose/accessors to accessors/basic and accessors-loose/basic

tim-mc · 2018-12-28T15:41:58Z

@nicolo-ribaudo Ok, updated with new & modified tests.

nicolo-ribaudo · 2019-01-06T20:05:23Z

packages/babel-plugin-proposal-private-methods/test/fixtures/accessors-loose/options.json

+    [
+      "external-helpers",
+      {
+        "helperVersion": "7.1.6"


Could you use something like 7.1000.0? So that we don't have to change this file if the helpers change in a futue version.

Sure, done.

nicolo-ribaudo

🎉

littledan · 2019-01-07T23:26:11Z

🎉 🎉

hzoo · 2019-01-16T17:38:29Z

packages/babel-plugin-proposal-private-methods/test/fixtures/accessors-loose/options.json

+    ["proposal-class-properties", { "loose": true }],
+    "transform-classes",
+    "transform-block-scoping",
+    "syntax-class-properties"


Not specific to this test/PR but was wondering if we actually need to include these plugins?

"transform-block-scoping", "syntax-class-properties"

xtuc · 2019-01-17T16:23:22Z

packages/babel-helper-create-class-features-plugin/src/fields.js

+    const isPrivate = prop.isPrivate();
+    const isMethod = !prop.isProperty();
+    const isInstance = !prop.node.static;
+    if (isPrivate) {


isPrivate seems only used here? Could you please inline it?

xtuc · 2019-01-17T16:25:08Z

packages/babel-helper-create-class-features-plugin/src/fields.js

+        initNodes.push(template.statement.ast`var ${id} = new WeakMap();`);
+      } else {
+        initNodes.push(template.statement.ast`var ${id} = new WeakSet();`);
+      }


The only difference is WeakMap vs WeakSet. Could we switch to an AST here and just parametrize that?

xtuc · 2019-01-17T16:25:19Z

packages/babel-helper-create-class-features-plugin/src/fields.js

+        initNodes.push(template.statement.ast`var ${id} = new WeakSet();`);
+      }
    } else if (!isStatic) {
      initNodes.push(template.statement.ast`var ${id} = new WeakMap();`);


See comment above

xtuc · 2019-01-17T16:26:45Z

packages/babel-helper-create-class-features-plugin/src/fields.js

+        });
+      `;
+    }
+  }


This looks quite verbose to me, could we switch it to building an AST and parametrizing the set/get? Also, I believe we have a few helpers that we could use here.

hzoo · 2019-01-21T23:30:02Z

Released in 7.3! Thanks everyone https://twitter.com/NicoloRibaudo/status/1087473662696017922?s=20

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from 1af57eb to d9593cb Compare November 29, 2018 19:25

danez added Spec: Class Fields Spec: Private Methods labels Nov 29, 2018

nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Nov 29, 2018

nicolo-ribaudo reviewed Nov 29, 2018

View reviewed changes

...babel-plugin-proposal-private-methods/test/fixtures/private-method-loose/accessors/output.js Outdated Show resolved Hide resolved

nicolo-ribaudo self-requested a review November 29, 2018 23:18

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from d9593cb to f5fcaf0 Compare December 3, 2018 23:10

tim-mc mentioned this pull request Dec 13, 2018

Update progress of Babel Implementation tc39/proposal-private-methods#53

Closed

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from f5fcaf0 to ee8b26f Compare December 24, 2018 19:28

tim-mc force-pushed the private-class-methods-stage-3_accessors branch 3 times, most recently from c145699 to d074dcc Compare December 24, 2018 21:11

nicolo-ribaudo reviewed Dec 27, 2018

View reviewed changes

packages/babel-helper-create-class-features-plugin/src/index.js Outdated Show resolved Hide resolved

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from d074dcc to b6427cc Compare December 27, 2018 16:05

nicolo-ribaudo reviewed Dec 27, 2018

View reviewed changes

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from b6427cc to 1cbd79a Compare December 27, 2018 23:16

nicolo-ribaudo mentioned this pull request Dec 29, 2018

Add support for decorators with private elements - Spec mode #9154

Closed

3 tasks

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from a2e4a79 to 85d2c6f Compare January 4, 2019 14:19

nicolo-ribaudo reviewed Jan 6, 2019

View reviewed changes

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from 85d2c6f to 74bbad2 Compare January 7, 2019 16:51

nicolo-ribaudo approved these changes Jan 7, 2019

View reviewed changes

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from 74bbad2 to e1b2bcb Compare January 11, 2019 21:32

loganfsmyth self-requested a review January 14, 2019 17:17

nicolo-ribaudo mentioned this pull request Jan 14, 2019

Babel 7.3.0 #9334

Closed

3 tasks

hzoo requested a review from jridgewell January 16, 2019 17:37

hzoo reviewed Jan 16, 2019

View reviewed changes

tim-mc added 11 commits January 17, 2019 09:36

Add accessor loose support

af5db96

Add private accessors spec support

621828a

Fix private dupe name check

9e41394

Changes from code review

5fcbd17

Add duplicated names tests

83c135c

Add get/set-only tests

a5d2172

Move accessors tests

f0d6486

Split out updates tests

2498ec2

Add helper change tests

4a328b1

Update test output

d6ccba0

Update test options

62a0ca4

tim-mc force-pushed the private-class-methods-stage-3_accessors branch from e1b2bcb to 62a0ca4 Compare January 17, 2019 15:05

xtuc reviewed Jan 17, 2019

View reviewed changes

hzoo approved these changes Jan 17, 2019

View reviewed changes

nicolo-ribaudo merged commit e8de6fa into babel:master Jan 21, 2019

This was referenced Jan 22, 2019

Private Methods (and accessors) (Stage 3) babel/proposals#22

Open

Private Static Methods (Stage 3) babel/proposals#55

Closed

tim-mc mentioned this pull request Mar 13, 2019

Private Static Accessors (Stage 3) babel/proposals#56

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

Uh oh!

Conversation

tim-mc commented Nov 29, 2018

Uh oh!

babel-bot commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-mc commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-mc commented Dec 24, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolo-ribaudo commented Dec 27, 2018

Uh oh!

tim-mc commented Dec 27, 2018

Uh oh!

nicolo-ribaudo commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-mc commented Dec 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

littledan commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Jan 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

babel-bot commented Nov 29, 2018 •

edited

Loading

tim-mc commented Nov 29, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 29, 2018 •

edited

Loading

nicolo-ribaudo commented Dec 27, 2018 •

edited

Loading