Private class methods stage 3 #8654

tim-mc · 2018-09-09T00:47:52Z

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

cc: @rricard @robpalme @littledan @syg

babel-bot · 2018-09-09T00:54:14Z

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

littledan

Great to see this initial milestone! Some initial comments. (Your list of TODOs looks good to me in addition.)

littledan · 2018-09-10T15:10:24Z

packages/babel-types/src/definitions/experimental.js

@@ -131,6 +134,28 @@ defineType("ClassPrivateProperty", {
  },
 });

+defineType("ClassPrivateMethod", {
+  builder: ["kind", "key", "params", "body", "computed", "static"],


Is "computed" needed?

I believe that I put that there because computed is a field (from classMethodOrDeclareMethodCommon). Should it not be here?

Yah, we need to remove it.

packages/babel-generator/test/fixtures/types/ClassBody-MethodDefinition/input.js

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

littledan · 2018-09-10T19:28:00Z

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

+    get(member) {
+      const { map, file } = this;
+
+      return t.callExpression(file.addHelper("classPrivateMethodGet"), [


How will this handle getters?

packages/babel-helpers/src/helpers.js

nicolo-ribaudo · 2018-09-14T20:54:53Z

class A {
  #method() {}
  
  getMethod() {
    return this.#method;
  }
}

console.log(new A().getMethod() === new A().getMethod());

Is this true or false?

littledan · 2018-09-17T15:48:20Z

@nicolo-ribaudo That should be true--they should all have the same function object as the method.

tim-mc · 2018-10-17T14:24:51Z

I've made some updates to switch to a WeakSet implementation (vs. the previous WeakMap implementation).

One issue I'm still working on is a potential variable name collision that can occur in the following situation (taking from the exec test located here):

class Foo {
  constructor(status) {
    this.status = status;
  }

  #getStatus() {
    return this.status;
  }

  getFakeStatus(fakeStatus) {
    const getStatus = this.#getStatus;
    return function () {
      return getStatus.call({ status: fakeStatus });
    };
  }
}

In the getFakeStatus method, a variable is created with the same name as the private method. This conflicts with the name of the variable assigned to the method that is defined in the outer scope, which looks like:

var getStatus = function getStatus() {
  return this.status;
};

...and will lead to this error:

TypeError: Cannot read property 'call' of undefined

I'm working on getting this resolved now. My current thinking is to generate a unique name for the function defined in the outer scope that is then referenced elsewhere.

Separately, I've noticed in the REPL for my builds that the helper functions are not getting updated with my most recent changes. Not sure if I'm doing something wrong...

tim-mc · 2018-10-25T17:39:20Z

I've just pushed up a fix for the name collision issue that I described earlier in #8654 (comment)

tim-mc · 2018-11-02T15:46:58Z

@nicolo-ribaudo @jridgewell I'm working on the loose implementation currently, but would be good to get some eyes on the re-worked WeakSet implementation I've pushed for spec.

It appears as though the issue I mentioned in my earlier comment re: helpers not getting updated in the REPL has resolved itself somehow. On a related note, I am curious what I should be putting as the version here (and how exactly that works).

Also, in terms of the tests that I've added, is the current set sufficiently exhaustive for the purpose of this PR (given that the additions should also be passing 262 tests)?

tim-mc · 2018-11-15T13:00:11Z

Due to a pending refactor PR (#8130), this PR is currently blocked. A WIP PR that takes the refactor into account has been opened here to garner feedback until the refactor has been merged. Once that happens, I will update this PR with the new changes.

tim-mc · 2018-11-24T00:22:08Z

Now that #8130 has landed, I've updated this branch to take those changes into account. I've also added support for loose mode. @nicolo-ribaudo @jridgewell @littledan

nicolo-ribaudo

Overall looks pretty good, but I think that we should throw if privateFields' loose !== privateMethods' loose

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

nicolo-ribaudo · 2018-11-24T12:15:51Z

packages/babel-plugin-class-features/src/features.js

+    throw path.buildCodeFrameError("Class private methods are not enabled.");
+  }
+
+  if (path.isClassPrivateMethod() && path.node.static) {


We should also guard agains private accessors.

Wrong button, I didn't mean to approve 😅

packages/babel-plugin-proposal-private-methods/test/fixtures/private-method/exfiltrated/exec.js

`writable` is false by default.

tim-mc · 2018-11-26T19:04:43Z

I have private method accessors ready in a separate branch - is it preferred that I combine that with this PR? @nicolo-ribaudo

nicolo-ribaudo · 2018-11-26T19:14:49Z

Nope, let's keep this as-is to make it easy to be reviewed.

jridgewell · 2018-11-28T22:11:16Z

packages/babel-plugin-class-features/src/features.js

+    }
+
+    if (path.node.static) {
+      throw new Error(


path.buildCodeFrameError

jridgewell · 2018-11-28T22:11:24Z

packages/babel-plugin-class-features/src/features.js

+    }
+
+    if (path.node.kind !== "method") {
+      throw new Error(


path.buildCodeFrameError.

jridgewell · 2018-11-28T22:12:03Z

packages/babel-plugin-class-features/src/features.js

+    hasFeature(file, FEATURES.fields) &&
+    isLoose(file, FEATURES.privateMethods) !== isLoose(file, FEATURES.fields)
+  ) {
+    throw new Error(


path.buildCodeFrameError

jridgewell · 2018-11-28T22:13:56Z

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

-    // In loose mode, both static and instance fields hare transpiled using a
+  for (const [
+    name,
+    { id, static: isStatic, method: isMethod },


Nit: Let's move this inside the for loop instead of making this uglier.

jridgewell · 2018-11-28T22:24:22Z

packages/babel-types/src/definitions/experimental.js

@@ -131,6 +134,28 @@ defineType("ClassPrivateProperty", {
  },
 });

+defineType("ClassPrivateMethod", {
+  builder: ["kind", "key", "params", "body", "computed", "static"],


Yah, we need to remove it.

tim-mc · 2018-11-28T23:49:45Z

Thanks @jridgewell, pushed up changes.

nicolo-ribaudo · 2018-11-29T05:51:41Z

Thank you very much 🎉

jaredatron · 2018-11-29T16:58:52Z

https://babeljs.io/repl/build/9484/ doesnt seem to work with private methods

nicolo-ribaudo · 2018-11-29T22:57:36Z

@deadlyicon The new plugin needs to be added to babel-standalone. Do you want to open a PR?

jaredatron · 2018-11-30T00:38:33Z

@deadlyicon The new plugin needs to be added to babel-standalone. Do you want to open a PR?

I'm just suggesting that this comment #8654 (comment) be changed so it is more clear on how to play with this new feature. I couldn't get it to work.

thanks :)

littledan reviewed Sep 10, 2018

View reviewed changes

tim-mc force-pushed the private-class-methods-stage-3 branch from 2fcfa3a to 5565a78 Compare September 10, 2018 19:52

nicolo-ribaudo reviewed Sep 14, 2018

View reviewed changes

packages/babel-helpers/src/helpers.js Outdated Show resolved Hide resolved

tim-mc force-pushed the private-class-methods-stage-3 branch from 5565a78 to f3b5369 Compare September 20, 2018 16:03

tim-mc force-pushed the private-class-methods-stage-3 branch 2 times, most recently from 0204bd5 to dfd0bb1 Compare October 14, 2018 19:53

tim-mc force-pushed the private-class-methods-stage-3 branch 2 times, most recently from 6248bb3 to ca44ffd Compare October 25, 2018 17:38

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

tim-mc force-pushed the private-class-methods-stage-3 branch 3 times, most recently from 2be5258 to 91acdc7 Compare November 2, 2018 14:30

Lodin mentioned this pull request Nov 3, 2018

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

Open

nicolo-ribaudo mentioned this pull request Nov 8, 2018

Merge class features plugins #8130

Merged

5 tasks

tim-mc mentioned this pull request Nov 14, 2018

[WIP] Private Class Methods Refactor nicolo-ribaudo/babel#1

Closed

tim-mc force-pushed the private-class-methods-stage-3 branch 2 times, most recently from 72cf153 to fdabc63 Compare November 24, 2018 00:12

nicolo-ribaudo added this to In progress in Class features Nov 24, 2018

nicolo-ribaudo previously approved these changes Nov 24, 2018

View reviewed changes

nicolo-ribaudo reviewed Nov 24, 2018

View reviewed changes

packages/babel-plugin-proposal-private-methods/test/fixtures/private-method/exfiltrated/exec.js Show resolved Hide resolved

nicolo-ribaudo added the Spec: Private Methods label Nov 24, 2018

tim-mc added 5 commits November 26, 2018 07:27

Add more isStatic & isMethod checks

59cd031

Remove writable:false from private method inits

6d8b4e4

`writable` is false by default.

Add private method func obj equality check

4b6cca8

Throw if private accessor is used

465810c

Add check for fields === private method loose mode

a84882e

tim-mc force-pushed the private-class-methods-stage-3 branch from 0c42842 to a84882e Compare November 26, 2018 19:10

jridgewell reviewed Nov 28, 2018

View reviewed changes

tim-mc added 3 commits November 28, 2018 18:47

Throw buildCodeFrameErrors instead of Errors

3e5f1a4

Move obj destructuring inside for loop

2cba230

Remove "computed" from ClassPrivateMethod type def

9b0e842

jridgewell approved these changes Nov 29, 2018

View reviewed changes

jridgewell merged commit 0859535 into babel:master Nov 29, 2018

Class features automation moved this from In progress to Done Nov 29, 2018

tim-mc mentioned this pull request Nov 29, 2018

Private Class Methods Stage 3: Private Accessors #9101

Merged

nicolo-ribaudo mentioned this pull request Dec 2, 2018

Add @babel/plugin-proposal-private-methods to @babel/standalone #9115

Merged

tim-mc mentioned this pull request Jan 28, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private class methods stage 3 #8654

Private class methods stage 3 #8654

tim-mc commented Sep 9, 2018 •

edited

babel-bot commented Sep 9, 2018 •

edited

littledan left a comment

littledan Sep 10, 2018

tim-mc Sep 10, 2018

jridgewell Nov 28, 2018

littledan Sep 10, 2018

nicolo-ribaudo commented Sep 14, 2018

littledan commented Sep 17, 2018

tim-mc commented Oct 17, 2018 •

edited

tim-mc commented Oct 25, 2018

tim-mc commented Nov 2, 2018

tim-mc commented Nov 15, 2018

tim-mc commented Nov 24, 2018 •

edited

nicolo-ribaudo left a comment

nicolo-ribaudo Nov 24, 2018

tim-mc Nov 24, 2018

tim-mc commented Nov 26, 2018

nicolo-ribaudo commented Nov 26, 2018

jridgewell Nov 28, 2018

jridgewell Nov 28, 2018

jridgewell Nov 28, 2018

jridgewell Nov 28, 2018

jridgewell Nov 28, 2018

tim-mc commented Nov 28, 2018

nicolo-ribaudo commented Nov 29, 2018

jaredatron commented Nov 29, 2018

nicolo-ribaudo commented Nov 29, 2018

jaredatron commented Nov 30, 2018

Private class methods stage 3 #8654

Private class methods stage 3 #8654

Conversation

tim-mc commented Sep 9, 2018 • edited

babel-bot commented Sep 9, 2018 • edited

littledan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Sep 14, 2018

littledan commented Sep 17, 2018

tim-mc commented Oct 17, 2018 • edited

tim-mc commented Oct 25, 2018

tim-mc commented Nov 2, 2018

tim-mc commented Nov 15, 2018

tim-mc commented Nov 24, 2018 • edited

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-mc commented Nov 26, 2018

nicolo-ribaudo commented Nov 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-mc commented Nov 28, 2018

nicolo-ribaudo commented Nov 29, 2018

jaredatron commented Nov 29, 2018

nicolo-ribaudo commented Nov 29, 2018

jaredatron commented Nov 30, 2018

tim-mc commented Sep 9, 2018 •

edited

babel-bot commented Sep 9, 2018 •

edited

tim-mc commented Oct 17, 2018 •

edited

tim-mc commented Nov 24, 2018 •

edited