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
Private Class Methods Stage 3: Private Accessors #9101
Conversation
1af57eb
to
d9593cb
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9811/ |
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,
If they should not be throwing, what would be the proper way to handle these? |
...babel-plugin-proposal-private-methods/test/fixtures/private-method-loose/accessors/output.js
Outdated
Show resolved
Hide resolved
Can't
Btw, I think that these cases are easier if you rely on the existing fields descriptor handling logic. |
d9593cb
to
f5fcaf0
Compare
f5fcaf0
to
ee8b26f
Compare
@nicolo-ribaudo I've made the updates to the PR that we discussed. |
c145699
to
d074dcc
Compare
packages/babel-helper-create-class-features-plugin/src/index.js
Outdated
Show resolved
Hide resolved
d074dcc
to
b6427cc
Compare
} | ||
|
||
descriptor.value = value; | ||
return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we usually use .filter(Boolean)
packages/babel-helper-create-class-features-plugin/src/index.js
Outdated
Show resolved
Hide resolved
This PR looks almost ready! |
b6427cc
to
1cbd79a
Compare
@nicolo-ribaudo requested changes have been made. |
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:
|
@nicolo-ribaudo Ok, updated with new & modified tests. |
a2e4a79
to
85d2c6f
Compare
[ | ||
"external-helpers", | ||
{ | ||
"helperVersion": "7.1.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
85d2c6f
to
74bbad2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
🎉 🎉 |
74bbad2
to
e1b2bcb
Compare
["proposal-class-properties", { "loose": true }], | ||
"transform-classes", | ||
"transform-block-scoping", | ||
"syntax-class-properties" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to this test/PR but was wondering if we actually need to include these plugins?
"transform-block-scoping", "syntax-class-properties"
e1b2bcb
to
62a0ca4
Compare
const isPrivate = prop.isPrivate(); | ||
const isMethod = !prop.isProperty(); | ||
const isInstance = !prop.node.static; | ||
if (isPrivate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
}); | ||
`; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Released in 7.3! Thanks everyone https://twitter.com/NicoloRibaudo/status/1087473662696017922?s=20 |
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