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
destructuring private fields with array pattern / object pattern #10017
destructuring private fields with array pattern / object pattern #10017
Conversation
tanhauhau
commented
May 23, 2019
Q | A |
---|---|
Fixed Issues? | Fixes #9310 |
Patch: Bug Fix? | 👍 |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
I guess I need to make sure it works on loose mode as well. |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11129/ |
@@ -100,6 +100,26 @@ const handle = { | |||
return; | |||
} | |||
|
|||
// { KEY: MEMBER } = OBJ -> { KEY: _ref } = OBJ; _set(MEMBER, _ref); | |||
// [MEMBER] = ARR -> [_ref] = ARR; _set(MEMBER, _ref); | |||
if ( |
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.
If I remember correctly, this will affect super.foo
expressions, too. Can you add a test for assigning [super.foo] = array
, too?
scope.push({ id: ref }); | ||
|
||
const assignmentParent = parentPath.getStatementParent(); | ||
assignmentParent.insertAfter( |
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.
I'm not sure we can insert after the statement, because later patterns can default to the property we're assigning:
[
this.#foo,
x = this.#foo
] = [0];
Should have x === 0
. The only way I can see this working in 100% of cases is if we use a special setter property:
// input
[this.#foo, x = this.#foo] = [0];
// output
const _this = this;
const internal = { set foo(v) { _this.#foo = v } };
[internal.foo, x = this.#foo] = [0]
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.
so the order matters for array pattern right?
eg:
[this.#foo, x = this.#foo] = [0]
x === 0
vs
this.#foo = 1;
[x = this.#foo, this.#foo] = [0];
x === 1
?
how about object destructuring?
this.#foo = 2;
{
x = this.#foo,
y: this.#foo,
z = this.#foo,
} = { y: 1 }
so x === 2
and z === 1
?
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.
Yup, you're correct in both cases.
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.
would it be better if we defined const internal = { set foo(v) { _this.#foo = v } };
at the constructor if we know there will be a destructuring, and reuse it throughout the scope of the class?
or else we will be keep creating internal
every time we see a destructuring pattern + default
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.
Yah, I would only create the object when we hit a destructure.
Depending on the order things are evaluated (need to check), we might be able to out things in the next property's key: { x: this.#foo, y = this.#foo } = {}
// ->
{ x = tmp, [this.#foo = tmp, "y"] = this.#foo } = {} Also, when we are destructing to member expressions instead of plain identifiers, this might work: [ this.#foo, foo.bar = this.#foo ] = [];
// ->
[ tmp, (this.#foo = tmp, foo).bar ] = [] I'm not sure about those optimizations, but I'd like to avoid using the setter when possible. |
I'm not sure I understand your input/output. Where does the array destructure come from in There can also be indirection between a default value and the private through a getter: class Ex {
get() {
return this.#foo;
},
destructure(arr) {
[ this.#foo, bar = this.get() ] = arr;
}
} Or maybe the private is a setter that mutates some other state. |
It's a computed key. |
wow, @nicolo-ribaudo this is cool, never thought of able to use comma operator this way. |
so you are saying, if there's a default value, you can't eliminate the possibility of reading from the updated private variable right |
Currently this code: class A {
#a;
#b() {}
get #c() {}
set #d(a) {}
test() {
this.#a = 1;
this.#b = 2;
this.#c = 3;
this.#d = 4;
}
} Is transpiled to this: class A {
constructor() {
_d.set(this, { set: _set_d });
_c.set(this, { get: _get_c });
_b.add(this);
_a.set( writable: true, value: void });
}
test() {
_classPrivateFieldSet(this, _a, 1);
_classPrivateMethodSet();
_classPrivateMethodSet();
_classPrivateFieldSet(this, _d, 4);
}
} When using destruturing: class A {
#a;
#b() {}
get #c() {}
set #d(a) {}
test() {
[this.#a, this.#b, this.#c, this.#d] = [1, 2, 3, 4];
}
} It can be transpiled like this: class A {
constructor() {
_d.set(this, { set: _set_d });
_c.set(this, { get: _get_c });
_b.add(this);
_a.set( writable: true, value: void });
}
test() {
[
_classPrivateFieldDestrSet(this, _a).value,
_classPrivateMethodSet(),
_classPrivateMethodSet(),
_classPrivateFieldDestrSet(this, _d).value,
] = [1, 2, 3, 4];
}
} Where the export default function _classPrivateFieldDestrSet(receiver, privateMap) {
if (!privateMap.has(receiver)) {
throw new TypeError("attempted to set private field on non-instance");
}
var descriptor = privateMap.get(receiver);
if ("value" in descriptor) return descriptor;
if (!("__destrObj" in descriptor)) {
descriptor.__destrObj = Object.defineProperty({}, "value", descriptor);
}
return descriptor.__destrObj;
} PS. My previous example was wrong; it should have been this: { x: tmp, [(this.#foo = tmp, "y")]: y = this.#foo } = {} |
Ha, I couldn't parse the computed key when it was inline with the other keys... 😅 That's an obscure desugaring, but I guess it'll work for object patterns. But array destructures can't use computed keys. I don't see a way to make this work in every case unless we use the setter objects. And if we have to maintain two ways to do it, why not just choose works-with-everything approach? Hopefully this will be rare enough to not matter (I didn't even realize you could destructure to a |
Yeah, in the last comment I posted another example which doesn't rely on computed keys (since I'm not even sure about the order in which they are evaluated). |
Sweet, just read the other comment. Let's use the private map's descriptor. Though, I don't understand the |
In case of a setter, the decriptor looks like this: const desc = {
get: _fn_get,
set: _fn_set,
} We can't just return the descriptor, because assigning to const objWithSetter = {
value: // This has the "descriptor" descriptor.
} I stored it in |
Ooo, clever. But the The helper approach is definitely simpler, I like it the most. export default function _classPrivateFieldDestructureSet(receiver, privateMap) {
if (!privateMap.has(receiver)) {
throw new TypeError("attempted to set private field on non-instance");
}
var descriptor = privateMap.get(receiver);
if ("value" in descriptor) return descriptor;
if (!("__destrObj" in descriptor)) {
descriptor.__destrObj = {
set value(v) {
descriptor.set.call(receiver, v)
},
};
}
return descriptor.__destrObj;
} |
Good catch, you're right! |
cool let me work on it! |
@nicolo-ribaudo i noticed that the define descriptor could be non |
b85bf22
to
29b6f54
Compare
@jridgewell Ok, I've read through the
So, I am thinking maybe I can
class A {
foo() {
([super.bar] = [1]);
}
}
// transpiled to
class A {
foo() {
([_ref] = [1]);
super.bar = _ref;
}
}
class A {
foo() {
([super.bar, b = super.bar] = [1, 2]);
}
}
// transpiled to
class A {
foo() {
const _this = this;
const temp = {
set value(_val) {
babelHelpers.set(
babelHelpers.getPrototypeOf(_obj),
"bar",
_val,
_this,
true
)
}
};
([temp.value, b = super.bar] = [1, 2]);
}
} what do you think? |
value: void 0 | ||
}); | ||
babelHelpers.classPrivateFieldLooseBase(this, _client)[_client] = 1; | ||
; |
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.
Where does this extra semicolon come from?
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.
oh, it's due to an extra comma in the input file
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.
removed extra comma
We’ll have to discuss how this interacts with super properties, for now it’d be fine to just throw an error in the transformer. |
4648b65
to
692eee3
Compare
(parentPath.isObjectProperty({ value: node }) && | ||
parentPath.parentPath.isObjectPattern()) || | ||
parentPath.isArrayPattern() || | ||
parentPath.isRestElement() |
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.
Do we also need to check for AssignmentPattern
s?
[this.#foo = 2] = []
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.
checked and added more test case
// { KEY: MEMBER = _VALUE } = OBJ | ||
(parentPath.isAssignmentPattern({ left: node }) && | ||
parentPath.parentPath.isObjectProperty({ value: parent }) && | ||
parentPath.parentPath.parentPath.isObjectPattern()) || |
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: I think we can actually join a few of the conditions here. parentPath.isAssignmentPattern()
is sufficient to check for both array pattern and object pattern, we don't need to consult the grand parent.
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.
i guess so, it's just to make my intentions clearer, not sure will assignment pattern be a child of not ust ObjectPattern
and ArrayPattern
?
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 can move these checks to be assertions: (t.assertObjectProperty(parentPath.parentPath, { value: parent })
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.
@nicolo-ribaudo what is the rationale behind using assertions
instead of is
?
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.
Just that with an assertion is clear that if this is an assignment expression, the parent will always be an object/array destructing
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.
Oh ok nevermind, we can't assert two different parent types.
(parentPath.isAssignmentPattern({ left: node }) && | ||
parentPath.parentPath.isArrayPattern()) || | ||
// {...MEMBER} | ||
// [...MEMBER] |
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.
Man, some of these syntaxes are crazy.
"ObjectPattern", | ||
"ArrayPattern", | ||
"MemberExpression", | ||
"CallExpression", |
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.
How did CallExpression
get into this?
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.
[_destructureSet(MEMBER) = _VALUE] = ARR
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.
That's invalid, isn't it? Chrome throws a syntax error.
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.
Yes you are right. my bad. fixed it
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.
LGTM
…l#10017) * destructuring private fields with array pattern / object pattern * wip: new test cases * destrucuring and rest for private properties * test case for loose private-loose * add transform-desturcturing for exec * update test case * remove getPrototypeOf imports from get and set * wip: destructure super assignment * throw "Destructuring to a super field is not supported yet." * fix tests and fix assignment pattern * remove CallExpression from AssignmentPattern