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
[WIP] Private Class Methods Refactor #1
[WIP] Private Class Methods Refactor #1
Conversation
32b5a65
to
93f2a89
Compare
I think that private methods should be transpiled exactly like private methods, because I could overwrite them. e.g. class A {
#x() { return 2 }
}
// ...
let _xMethod = function () { return 2 };
class A {
constructor() {
_x.set(this, {
writable: true,
value: _xMethod
});
}
}
var _x = new WeakMap(); This is because methods could be overwritten exactly as if they were fields, e.g. class A {
#x() { return 2 }
setX() { this.#x = "foo"; }
} |
@nicolo-ribaudo re: #1 (comment), private methods are not writable. Your second code snippet should result in a |
Oh right. I still think that we should use weakmaps with descriptors, since decorators (I will implement support for decorated private elements after that this PR is merged) can make private methods writable, or change them to be fields at runtime. Having a single way of accessing private things makes it a lot easier. |
The main factor behind rewriting the IIRC the performance cost of using Given the above, is it possible to continue with the |
9651072
to
1d08d59
Compare
I think we could use WeakSet for private methods by themselves, and move to WeakMap if needed when decorators are in use. It is very unfortunate to have per-instance, per-method space overhead! The WeakSet representation has the potential to eliminate this by using the same set for a group of methods. |
Ok we can go ahead with weak sets. If we see that having to have a different path for decorators makes things too complex we can always revisit the decision later. |
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.
Can we make this plugin throw when it finds a static private method or a private accessor, since they are not supported?
const { params, body } = prop.node; | ||
const methodValue = prop.node | ||
? t.functionExpression(methodId, params, body) | ||
: prop.scope.buildUndefinedNode(); |
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.
When can this be undefined
?
const { | ||
staticNodes, | ||
instanceNodes, | ||
declarationNodes, |
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.
Can't declarationNodes
go with staticNodes
?
parserOpts.plugins.push( | ||
"classProperties", | ||
"classPrivateProperties", | ||
"classPrivateMethods", |
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.
We should have a @babel/plugin-syntax-private-methods
for this, parallel to @babel/plugin-proposal-private-methods
.
e70d0f0
to
5114047
Compare
This is a refactor of babel#8654 that implements private method support on top of babel#8130.