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: Private Accessors #9101

Merged

Conversation

@tim-mc
Copy link
Contributor

tim-mc commented Nov 29, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from 1af57eb to d9593cb Nov 29, 2018
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Nov 29, 2018

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

@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Nov 29, 2018

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?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 29, 2018

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.

@nicolo-ribaudo nicolo-ribaudo self-requested a review Nov 29, 2018
@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from d9593cb to f5fcaf0 Dec 3, 2018
@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from f5fcaf0 to ee8b26f Dec 24, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Dec 24, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch 3 times, most recently from c145699 to d074dcc Dec 24, 2018
@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from d074dcc to b6427cc Dec 27, 2018
}
descriptor.value = value;
return value;

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Dec 27, 2018

Member

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

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

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Dec 27, 2018

Member

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

@@ -404,7 +535,7 @@ export function buildFieldsInitNodes(

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

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Dec 27, 2018

Member

Nit: we usually use .filter(Boolean)

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 27, 2018

This PR looks almost ready!

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from b6427cc to 1cbd79a Dec 27, 2018
@tim-mc

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Dec 27, 2018

@nicolo-ribaudo requested changes have been made.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 27, 2018

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

This comment has been minimized.

Copy link
Contributor Author

tim-mc commented Dec 28, 2018

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from a2e4a79 to 85d2c6f Jan 4, 2019
[
"external-helpers",
{
"helperVersion": "7.1.6"

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 6, 2019

Member

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.

This comment has been minimized.

Copy link
@tim-mc

tim-mc Jan 7, 2019

Author Contributor

Sure, done.

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from 85d2c6f to 74bbad2 Jan 7, 2019
Copy link
Member

nicolo-ribaudo left a comment

🎉

@littledan

This comment has been minimized.

Copy link

littledan commented Jan 7, 2019

🎉 🎉

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from 74bbad2 to e1b2bcb Jan 11, 2019
@loganfsmyth loganfsmyth self-requested a review Jan 14, 2019
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jan 14, 2019
3 of 3 tasks complete
@hzoo hzoo requested a review from jridgewell Jan 16, 2019
["proposal-class-properties", { "loose": true }],
"transform-classes",
"transform-block-scoping",
"syntax-class-properties"

This comment has been minimized.

Copy link
@hzoo

hzoo Jan 16, 2019

Member

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

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

@tim-mc tim-mc force-pushed the tim-mc:private-class-methods-stage-3_accessors branch from e1b2bcb to 62a0ca4 Jan 17, 2019
const isPrivate = prop.isPrivate();
const isMethod = !prop.isProperty();
const isInstance = !prop.node.static;
if (isPrivate) {

This comment has been minimized.

Copy link
@xtuc

xtuc Jan 17, 2019

Member

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

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

This comment has been minimized.

Copy link
@xtuc

xtuc Jan 17, 2019

Member

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

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

This comment has been minimized.

Copy link
@xtuc

xtuc Jan 17, 2019

Member

See comment above

});
`;
}
}

This comment has been minimized.

Copy link
@xtuc

xtuc Jan 17, 2019

Member

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
hzoo approved these changes Jan 17, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit e8de6fa into babel:master Jan 21, 2019
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.77% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Class features automation moved this from In progress to Done Jan 21, 2019
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jan 21, 2019

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

@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.