Class Private Static Accessors by tim-mc · Pull Request #10217 · babel/babel

tim-mc · 2019-07-14T00:37:04Z

Q	A
Fixed Issues?	babel/proposals#56, closes #8052
Patch: Bug Fix?	--
Major: Breaking Change?	--
Minor: New Feature?	Add class private static accessor support
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

cc @robpalme @nicolo-ribaudo @littledan

tim-mc · 2019-07-14T00:42:03Z

Still a WIP. Spec support has been added. Working on loose now.

babel-bot · 2019-07-14T00:48:23Z

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

tim-mc · 2019-07-15T15:52:37Z

Loose support has been added

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

JLHwung · 2019-07-18T14:02:59Z

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

-  const { id } = privateNamesMap.get(prop.node.key.id.name);
+  const privateName = privateNamesMap.get(prop.node.key.id.name);
+  const { id, getId, setId, initAdded } = privateName;
  const value = prop.node.value || prop.scope.buildUndefinedNode();


I suggest move this line immediately before where value is used so we can early return for non-accessor class private methods.

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

jridgewell · 2019-07-20T19:51:56Z

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

+  const privateName = privateNamesMap.get(prop.node.key.id.name);
+  const { id, getId, setId, initAdded } = privateName;
  const value = prop.node.value || prop.scope.buildUndefinedNode();
+  if (!prop.isProperty() && (initAdded || !(getId || setId))) return;


When would it not be a property?

It wouldn't be a property if it's a static private accessor.

Nit: could you extract getId || setId to an isAccessor boolean variable?
I find it hard to read all these nested boolean expressions.

jridgewell · 2019-07-20T19:52:51Z

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

+          // enumerable is false by default
+          // writable is false by default
+          get: ${getId.name},
+          set: ${setId.name}


Ditto reuse.

That works in this case, but setting null as a getter/setter here is a TypeError.

Ah, yah. We should be able to use undefined in all cases instead.

Good point, updated.

jridgewell · 2019-07-20T19:54:06Z

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

        staticNodes.push(
+          buildPrivateStaticFieldInitSpec(prop, privateNamesMap),
+        );
+        staticNodes.unshift(


Why unshift?

I use unshift so that the declaration of the getter and/or setter occurs before the usage of the accessor(s) (e.g., when they're defined as the values on an object). Here's what a test failure looks like if that's changed to push:

babel/packages/babel-plugin-proposal-private-methods/test/fixtures/static-accessors/basic/exec.js: expect(received).toEqual(expected) // deep equality Expected: "top secret string" Received: undefined 1 | class Cl { 2 | static getValue() { 3 | return babelHelpers.classStaticPrivateFieldSpecGet(Cl, Cl, _privateStaticFieldValue); 4 | } 5 | 6 | static setValue() { 7 | babelHelpers.classStaticPrivateFieldSpecSet(Cl, Cl, _privateStaticFieldValue, "dank"); 8 | } 9 | 10 | } 11 | 12 | var _PRIVATE_STATIC_FIELD = { 13 | writable: true, 14 | value: "top secret string" 15 | }; 16 | var _privateStaticFieldValue = { 17 | get: _get_privateStaticFieldValue, 18 | set: _set_privateStaticFieldValue 19 | }; 20 | 21 | var _get_privateStaticFieldValue = function () { 22 | return babelHelpers.classStaticPrivateFieldSpecGet(Cl, Cl, _PRIVATE_STATIC_FIELD); 23 | }; 24 | 25 | var _set_privateStaticFieldValue = function (newValue) { 26 | babelHelpers.classStaticPrivateFieldSpecSet(Cl, Cl, _PRIVATE_STATIC_FIELD, `Updated: ${newValue}`); 27 | }; 28 | 29 | expect(Cl.getValue()).toEqual("top secret string"); 30 | Cl.setValue(); 31 | expect(Cl.getValue()).toEqual("Updated: dank");

Can we change the get/set variables to function declarations instead? If not, this is still fine.

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

nicolo-ribaudo · 2019-07-25T08:04:40Z

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

+  const privateName = privateNamesMap.get(prop.node.key.id.name);
+  const { id, getId, setId, initAdded } = privateName;
  const value = prop.node.value || prop.scope.buildUndefinedNode();
+  if (!prop.isProperty() && (initAdded || !(getId || setId))) return;


Nit: could you extract getId || setId to an isAccessor boolean variable?
I find it hard to read all these nested boolean expressions.

tim-mc force-pushed the private-static-accessors branch from 1de0bb8 to 07420b3 Compare July 14, 2019 00:39

tim-mc marked this pull request as ready for review July 15, 2019 15:52

nicolo-ribaudo self-requested a review July 15, 2019 16:02

tim-mc force-pushed the private-static-accessors branch 2 times, most recently from a8def51 to 60b2fa9 Compare July 17, 2019 23:44

JLHwung reviewed Jul 18, 2019

View reviewed changes

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

JLHwung reviewed Jul 18, 2019

View reviewed changes

tim-mc force-pushed the private-static-accessors branch from 60b2fa9 to 14f480d Compare July 19, 2019 20:05

jridgewell approved these changes Jul 20, 2019

View reviewed changes

tim-mc force-pushed the private-static-accessors branch from 14f480d to ee9364e Compare July 22, 2019 13:48

jridgewell approved these changes Jul 22, 2019

View reviewed changes

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

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

nicolo-ribaudo approved these changes Jul 25, 2019

View reviewed changes

tim-mc force-pushed the private-static-accessors branch 2 times, most recently from 1bcccbd to 5ec8290 Compare July 29, 2019 15:51

nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Private Methods labels Aug 1, 2019

nicolo-ribaudo added this to the v7.6.0 milestone Aug 1, 2019

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Aug 1, 2019

JLHwung approved these changes Aug 1, 2019

View reviewed changes

tim-mc added 8 commits August 23, 2019 14:07

Remove error for static private accessors

4584068

Private static accessors strict

d0e94da

Add loose mode support

231861b

Move value decl for early return

15ad1b5

Reuse getter/setter template

6f7ed38

Reuse getter/setter templates

3f8d021

Use buildUndefinedNode in accessor templates

3e6c955

Extract isAccessor variable

fc24d84

tim-mc force-pushed the private-static-accessors branch from 5ec8290 to fc24d84 Compare August 23, 2019 19:59

nicolo-ribaudo merged commit 3e4889d into babel:master Sep 6, 2019

snyk-bot mentioned this pull request Oct 24, 2019

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.6.3 Matthelonianxl/drizzle#9

Open

snyk-bot mentioned this pull request Nov 1, 2019

[Snyk] Upgrade @babel/core from 7.2.2 to 7.6.4 nbv1982/material-ui#5

Open

snyk-bot mentioned this pull request Nov 12, 2019

[Snyk] Upgrade @babel/runtime from 7.1.5 to 7.6.3 ajesse11x/Parse-SDK-JS#1

Open

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

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

lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019

Uh oh!

Conversation

tim-mc commented Jul 14, 2019 • edited by nicolo-ribaudo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-mc commented Jul 14, 2019

Uh oh!

babel-bot commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-mc commented Jul 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

tim-mc Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tim-mc commented Jul 14, 2019 •

edited by nicolo-ribaudo

Loading

babel-bot commented Jul 14, 2019 •

edited

Loading

tim-mc Jul 22, 2019 •

edited

Loading