Class private properties by jridgewell · Pull Request #7842 · babel/babel

jridgewell · 2018-04-30T22:49:58Z

Supersedes #7555.

Let's see if we can land it this time.

babel-bot · 2018-04-30T22:56:19Z

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

loganfsmyth · 2018-04-30T23:19:50Z

One thing we talked about at the last Babel meeting was the possibility of merging the decorator transform into public/private, so we'd have a single one to cover

public fields
private fields
decorators

since it seems like, for decorators to work, especially with private fields, they are going to end up needing intricate knowledge of the output produced by the other transforms.

That's not to say that these would all by on by default, but you could imagine

plugins: [
  ['proposal-class-extensions', { publicFields: true }],
]

for instance only enabling public fields, for instance.

Decorators for instance are going to require ways to programmatically access private fields from within the decorator function, and it seems like it'd be essentially impossible if written as two separate, tightly-coupled transforms, especially when long-term maintenance will be important.

What are your thoughts on that, and is this PR merges private and public already, would now be a good time to do that plugin rename to get away from the "class properties" name?

nicolo-ribaudo

It throws 🤔

https://babeljs.io/repl/build/7810/#?babili=false&browsers=&build=&builtIns=false&code_lz=MYGwhgzhAECC0G8BQ1oGIBmB7LBuF0BAtgKYAuAFlgCYAUAlIgapQJYQB0mO-qAvkj5A&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&lineWrap=true&presets=react%2Cstage-1&prettier=false&targets=&version=7.0.0-beta.46%2Bpr.7842&envVersion=

class A {
  #foo;
  
  method() {
    this.#foo;
  }
}

Method WeakMap.prototype.has called on incompatible receiver #

nicolo-ribaudo · 2018-05-02T16:41:26Z

packages/babel-helper-member-expression-to-functions/README.md

  },

-  memoize(memberPath) {
+  memoise(memberPath) {


Google disagrees 😛

maybeGenerateMemoised. @kittens uses British spellings. Personally, it's all wrong.

It's fine to change it 😛if it's easier for everyone

Followup PR. For now, let's keep the codebase consistent.

nicolo-ribaudo · 2018-05-02T16:48:36Z

packages/babel-helpers/src/helpers.js

+
+helpers.classPrivateFieldKey = () => template.program.ast`
+  var id = 0;
+  export default function _classPrivateFieldKey(name) {


Could you add Loose to the names of these two helpers?

nicolo-ribaudo · 2018-05-02T16:55:24Z

packages/babel-plugin-proposal-class-properties/src/index.js

+      const { name } = this;
+      const { node, parentPath } = path;
+
+      if (!parentPath.isMemberExpression({ property: node })) return;


When can this happen?

The private name of a private field/method. Also if we ever allow bare privates.

nicolo-ribaudo · 2018-05-02T18:27:50Z

packages/babel-plugin-proposal-class-properties/src/index.js

-          const propNode = prop.node;
-          if (propNode.decorators && propNode.decorators.length > 0) continue;
+          const inits = [];
+          privateInits.push(inits);


Nit: this two lines can be moved inside the if statement.

Also, why can't these loops be merged?

Also, why can't these loops be merged?

The privates need to be transformed first in case a public field references one. I should probably test that a private field that references another transforms correctly...

jridgewell · 2018-05-04T05:41:46Z

Decorators for instance are going to require ways to programmatically access private fields from within the decorator function, and it seems like it'd be essentially impossible if written as two separate, tightly-coupled transforms, especially when long-term maintenance will be important.

Honestly, I'd rather the presence of a decorators go down a completely separate path, ripping out everything with one out of the class.

I imagine that'll make everything considerably easier, even if it does duplicate some of the transform in runtime.

Method WeakMap.prototype.has called on incompatible receiver #

Ugh, subclassing WeakMap. I'll update.

nicolo-ribaudo

The code looks good, apart from the WeakMap error.

nicolo-ribaudo · 2018-05-17T20:53:41Z

...es/babel-plugin-proposal-class-properties/test/fixtures/private-loose/canonical/options.json

@@ -0,0 +1,3 @@
+{
+  "minNodeVersion": "6.0.0"


This isn't needed, since we only run the tests on node >= 6

nicolo-ribaudo · 2018-05-17T20:58:13Z

packages/babel-plugin-proposal-class-properties/test/fixtures/private-loose/update/output.js

+  return Foo;
+}();
+
+var _foo = babelHelpers.classPrivateFieldLooseKey("foo");


Can we move these things inside the class closure? IIRC, @Andarist implemented something like @babel/helper-get-class-iife to do that.

Oh it has not been merged yet - #6963

jridgewell · 2018-05-17T21:14:17Z

Which weakmap error?

nicolo-ribaudo · 2018-05-17T21:35:15Z

Method WeakMap.prototype.has called on incompatible receiver #<AssignmentMemoiser>

1. Babel using British spellings, so `memoise` 2. Provide a helper `AssignmentMemoiser` class, which will assign the memo'd value with the `n`th access.

jridgewell · 2018-05-18T01:47:40Z

Ach, updated to remove the extends WeakMap.

SavePointSam · 2018-05-18T20:51:00Z

🎉

piranna · 2018-05-19T08:58:34Z

What version will this be published in npm? What will be the final semantics? Will it be available with babel-preset-env? How to enable support for this?

TrySound · 2018-05-19T15:52:11Z

@piranna The next one. The same as in spec. Nope, until any browser will implement it. Use stage-3 or class-properties plugin.

nicolo-ribaudo reviewed May 2, 2018

View reviewed changes

This was referenced May 6, 2018

Class private properties #7555

Closed

Tooling implementation status tc39/proposal-class-fields#57

Open

nicolo-ribaudo reviewed May 17, 2018

View reviewed changes

jridgewell added 8 commits May 17, 2018 21:42

Update member-expression-to-functions

1578880

1. Babel using British spellings, so `memoise` 2. Provide a helper `AssignmentMemoiser` class, which will assign the memo'd value with the `n`th access.

Private properties!

34acca7

Fixes

59442b3

Tests

9b6905e

Update helper name

77c0ded

Fix privates that reference other privates

211202d

Don't extend a builtin

6fee331

Rebase

cca792f

jridgewell force-pushed the class-private-properties branch from 98caf52 to cca792f Compare May 18, 2018 01:46

nicolo-ribaudo approved these changes May 18, 2018

View reviewed changes

nicolo-ribaudo mentioned this pull request May 18, 2018

Add support for the new decorators proposal #7976

Merged

4 tasks

jridgewell merged commit 27c39c5 into babel:master May 18, 2018

jridgewell deleted the class-private-properties branch May 18, 2018 18:03

imurchie mentioned this pull request May 22, 2018

Shift name of derivedData attribute appium/appium-xcuitest-driver#678

Merged

rricard mentioned this pull request May 25, 2018

Static Class Features: Stage 3 #8052

Closed

bakkot mentioned this pull request May 31, 2018

A summary of feedback regarding the # sigil prefix tc39/proposal-class-fields#100

Closed

joeywatts mentioned this pull request Oct 9, 2018

Emit "void 0" initializations for uninitialized class properties for ESNext compatibility joeywatts/TypeScript#5

Closed

joeywatts mentioned this pull request Oct 9, 2018

Allow pass through of class-properties microsoft/TypeScript#12212

Closed

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

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

Uh oh!

Conversation

jridgewell commented Apr 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

loganfsmyth commented Apr 30, 2018

Uh oh!

nicolo-ribaudo left a comment

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!

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!

jridgewell commented May 4, 2018

Uh oh!

nicolo-ribaudo left a comment

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!

jridgewell commented May 17, 2018

Uh oh!

nicolo-ribaudo commented May 17, 2018

Uh oh!

jridgewell commented May 18, 2018

Uh oh!

SavePointSam commented May 18, 2018

Uh oh!

piranna commented May 19, 2018

Uh oh!

TrySound commented May 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

babel-bot commented Apr 30, 2018 •

edited

Loading