-
Notifications
You must be signed in to change notification settings - Fork 113
Destructuring instances with private fields #4
Comments
I agree. FWIW, I have no problem with what you've proposed 👍 |
Well, this makes sense, but at the same time it sort of subsumes a possible semantics for private names in object literals. If we allow |
Good point. I'm not certain the use case for destructuring private fields is super necessary due to the shorthand anyway. Another reason I see people destructuring is to alias things to avoid variable name conflicts, but that's less of a problem with private fields... |
I'm not sure I follow:
Did you mean "not introduce a new binding for #foo on the left hand side"? I think I may be missing something, but AFAICT this works with future support for privates in objects too... const obj = {#p: 42, bar() {
const {#p: p} = obj;
return p;
}}; ( To be clear, I am not suggesting the following destructuring syntax since it would be problematic: (In theory we could remove this restriction for expression-patterns where there may actually be a private binding already -- but I'd be fine with staying strict there since that seems like a pretty obscure scenario and remaining congruent between expression and binding forms is nice) |
I meant on the right-hand side: What you're saying seems possible, but it seems strange that in an object literal as an expression, a new unique binding is introduced, but as destructuring it refers to an existing binding. But maybe this is obvious when you use it, and I'm just being a spec pedant. |
After a bit of conversation with @littledan I'm convinced that this could be confusing enough that it warrants further discussion and we should leave it to a separate follow-up proposal if ever it comes into demand. The example he posed that convinced me was: class X {
#a;
foo() {
let {#a: a} = {#a: b};
}
} There's nothing technically wrong with this example, but it may be confusing enough that someone could mistakenly expect that the |
Any further thoughts? I think the ability to destructure private fields should be possible and consistent with non-private fields:
and therefore this also makes complete sense and should not work @jeffmo
All in all, to me it looks like a reasonable thing to expect from a publicly inaccessible member and the hash sign makes it quite clear that this is expected. I think this would be a welcome feature and consistent with destructuring of public fields with known and expected limitations for private ones. For your consideration: class Thing {
#a = 1;
b = 2;
c = 3;
d = 4;
#z = null;
start = () => {
this.doStuff(this);
}
doStuff = ({c, d, b} = this) => {
console.log({b, d, c});
this.doThing(this);
}
doThing = ({d, c, b} = this) => {
console.log({b, c, d});
this.doMore(this);
}
noCanDo = ({b, d, c, #a, #z} = this) => {
console.log({d, c, e, #a, #z}); // won't work
console.log({d, c, e, a: this.#a, z: this.#z});
}
} |
Of course it won’t work, because you can’t declare a variable using # syntax. |
Well, even with the private fields aliased, that still works better than the alternative @ljharb noCanDo = ({b, d, c, #a: a, #z: z} = this) => {
console.log({d, c, e, a, z}); // would work
console.log({d, c, e, a: this.#a, z: this.#z});
} |
@littledan I just ran into this (using it in production because I'm psycho) and thought it was curious. Relevant Twitter thread here I had a few thoughts about this (I'm neither for or against it), and I didn't see them in this thread: On an elementary level, it seems like it would be safe to allow destructure from class Person {
#income = 100;
#ssn = '123-45-6789';
fileTaxes() {
const { #income: income, #ssn: ssn } = this;
irs.shenanigans(income, ssn);
}
} If it was allowed one unintended consequence of this would be the fact that destructuring can be done with a class Person {
#income = 100;
#ssn = '123-45-6789';
fileTaxes() {
const { ...what } = this;
irs.shenanigans(what['#income'], what['#ssn']); // This doesn't seem right.
}
} Maybe we could try to allow only named destructures of private fields, but that might confuse people as well (although less often). class Person {
#income = 100;
#ssn = '123-45-6789';
fileTaxes() {
const { #income: income, #ssn: ssn } = this; // okay
irs.shenanigans(income, ssn);
}
recordPrivateData() {
const { ...allPersonalData } = this;
record(allPersonalData); // #ssn and #income are missing!
}
} But this isn't entirely "new" behavior, because people can already destructure non-enumerable properties by name, but not by var obj = { prop1: 'one' };
Object.defineProperty(obj, 'prop2', { value: 'two', enumerable: false });
var { prop2 } = obj;
console.log(prop2); // "two";
var { ...copy } = obj;
console.log(copy); // { prop1: 'one' } I guess I'm indifferent about whether or not this ends up supported. However, I will say that it makes dealing with |
Yup: let o = Object.create(Object.prototype, { 'x': { value: 0, enumerable: true }, 'y': { value: 1, enumerable: false } });
console.log({...o}); // { x: 0 }
let { x: a, y: b } = o;
console.log(a, b); // 0, 1 I think allowing (I do want to say that I'm opposed to allowing a bare |
I think this syntax is good, and we should add it. This mirrors stringified destructuring |
FWIW: I think that the real catch here is that it should only be allowed from class X {
#a;
foo() {
let {#a: a} = {#a: b};
// ^ --- SyntaxError here I hope
}
bar() {
let { #a: a } = this; // okay
}
baz() {
let { ...copy } = this;
copy.#a; // SyntaxError copy isn't an instance of the enclosing class.
}
} @littledan maybe this issue could be reopened to track the discussion? Or at least so it stays in memory if there's a follow up proposal. Again, I don't have strong feelings about whether this should land, but it seems like it's a possibility? |
Accessing a private field is done at runtime, not syntax time. So you'd be able to destructure from random objects, but it would throw if the field isn't installed on the RHS object. |
@benlesh I don't think it makes sense to restrict what can go on the RHS, any more than it does to restrict what you can put before That said, foo() {
let {#a: a} = {#a: b};
} would still be a SyntaxError, but on the RHS rather than the LHS. |
I don't see your point. I guess I don't agree at all. If you had an identifier that pointed to an object that had a private variable, and it wasn't a reference to the class you were in, it wouldn't work. Which seems reasonable to me. It has little to do with the right hand side and has more to do with what you're accessing. |
Oh well. It seems decided. As I stated earlier I really don't care if this gets added. Haha. Although I really do think it could be added and it's a bit of a shame that it doesn't work given all the effort that went into destructuring work and how popular it is to destructure "private" fields off of classes and TypeScript. |
I've created https://github.com/jridgewell/proposal-destructuring-private to track this. |
I feel like the examples being pointed out are false equivalencies. Everyone is talking about object literals when I'm actually talking about |
@benlesh Your example had an object literal in it: I think we're mostly on the same page: |
It just occurred to me that, due to the nature of private-field names, destructuring private fields off an instance into a local binding is not possible with the spec as-is.
I think this could be fixed with a simple tweak to destructuring grammar, though:
would roughly desguar to
Should we include this destructuring grammar in this proposal? Or defer it to a separate proposal?
(I would be in favor of including it if it's as non-controversial as I imagine, defer if it's at all controversial)
The text was updated successfully, but these errors were encountered: