Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Private class methods stage 3 #8654

Merged
merged 12 commits into from Nov 29, 2018

Conversation

@tim-mc
Copy link
Contributor

tim-mc commented Sep 9, 2018

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

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Sep 9, 2018

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

Copy link

littledan left a comment

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

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

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

This comment has been minimized.

Copy link
@littledan

littledan Sep 10, 2018

Is "computed" needed?

This comment has been minimized.

Copy link
@tim-mc

tim-mc Sep 10, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

Yah, we need to remove it.

get(member) {
const { map, file } = this;

return t.callExpression(file.addHelper("classPrivateMethodGet"), [

This comment has been minimized.

Copy link
@littledan

littledan Sep 10, 2018

How will this handle getters?

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch from 2fcfa3a to 5565a78 Sep 10, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Sep 14, 2018

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

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

Is this true or false?

@littledan

This comment has been minimized.

Copy link

littledan commented Sep 17, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch from 5565a78 to f3b5369 Sep 20, 2018
@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch 2 times, most recently from 0204bd5 to dfd0bb1 Oct 14, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Oct 17, 2018

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 tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch 2 times, most recently from 6248bb3 to ca44ffd Oct 25, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Oct 25, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch 3 times, most recently from 2be5258 to 91acdc7 Oct 31, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 2, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 15, 2018

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 tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch 2 times, most recently from 72cf153 to fdabc63 Nov 24, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 24, 2018

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 nicolo-ribaudo added this to In progress in Class features Nov 24, 2018
Copy link
Member

nicolo-ribaudo left a comment

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

packages/babel-plugin-class-features/src/fields.js Outdated Show resolved Hide resolved
packages/babel-plugin-class-features/src/fields.js Outdated Show resolved Hide resolved
throw path.buildCodeFrameError("Class private methods are not enabled.");
}

if (path.isClassPrivateMethod() && path.node.static) {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Nov 24, 2018

Member

We should also guard agains private accessors.

This comment has been minimized.

Copy link
@tim-mc

tim-mc Nov 24, 2018

Author Contributor

Added!

@nicolo-ribaudo nicolo-ribaudo dismissed their stale review Nov 24, 2018

Wrong button, I didn't mean to approve 😅

@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 26, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3 branch from 0c42842 to a84882e Nov 26, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 26, 2018

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

}

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

path.buildCodeFrameError

}

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

path.buildCodeFrameError.

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

path.buildCodeFrameError

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

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

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

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

This comment has been minimized.

Copy link
@jridgewell

jridgewell Nov 28, 2018

Member

Yah, we need to remove it.

@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 28, 2018

Thanks @jridgewell, pushed up changes.

@jridgewell jridgewell merged commit 0859535 into babel:master Nov 29, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.62% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Class features automation moved this from In progress to Done Nov 29, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 29, 2018

Thank you very much 🎉

@deadlyicon

This comment has been minimized.

Copy link

deadlyicon commented Nov 29, 2018

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

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 29, 2018

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

@deadlyicon

This comment has been minimized.

Copy link

deadlyicon commented Nov 30, 2018

@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 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
6 participants
You can’t perform that action at this time.