tc39 / proposal-destructuring-private

A proposal integrate private fields and destructuring
https://tc39.es/proposal-destructuring-private/
MIT License
45 stars 9 forks source link

Stage 3 Criteria #2

Open jridgewell opened 2 years ago

jridgewell commented 2 years ago
rricard commented 2 years ago

Hi Justin, I checked your proposal against the stage 3 review document in "How We Work" and it looks good from my own knowledge and point of view.

  • Does the proposal address its stated motivation?

Yes it does as it adds destructuring to private fields in a very contained manner.

  • Is this language feature intuitive and learnable?

In my experience teaching colleagues how to use private fields, I have seen instances of them trying to destructure them. They would instinctively try the syntax described here so it looks like a "natural" addition to private fields.

  • Does the proposal fit in well when combined with other JavaScript language features (both current and proposed, a.k.a. "cross-cutting concerns")? Will the interactions between features be surprising or strange?

As said previously, this is not only very contained, but it makes sense if you intersect destructuring with private fields.

  • Does the proposal scope make sense, or would this make more sense as a larger or smaller proposal?

This is the minimum amount of scope this feature can do.

  • Does the specification text completely cover every aspect of the proposal, or are some things unstated?

I believe so, however I am not an editor and I did miss to notice #4 for instance.

  • Is the specification text logical and consistent, matching the rest of the JavaScript specification?

Likewise, I believe so, but an editor would be able to confirm this better than me.

  • Do all of the details and edge cases seem reasonably motivated?

There are actually few as most mechanisms described here rely on existing mechanisms linked to private fields and how they can be accessed. Nothing significantly new has been added here apart from the interaction with the destructuring syntax.

jridgewell commented 2 years ago

Thanks!

And sorry to the newbie reviewers, this proposal got a little bit bigger than I thought due to #3 and #4. 😬

bakkot commented 2 years ago

LGTM other than https://github.com/tc39/proposal-destructuring-private/pull/8.

ljharb commented 2 years ago

LGTM also

syg commented 2 years ago

lgtm % small editorial stuff we can fix up when merging into ecma262.

Some editorial stuff I noticed:

1. Let _x_ be ~empty~.
1. If foo,
    1. Set _x_ to bar.
1. Else,
    1. Set _x_ to baz.

... to be simplified to ...

1. If foo,
    1. Let _x_ be bar.
1. Else,
    1. Let _x_ be baz.
jridgewell commented 2 years ago

Ping @waldemarhorwat, I'm particularly interested in your review (though you weren't there for the confirmation of Stage 3 reviewers in the last meeting, I kept you on the as a reviewer. Feel free to say you don't wish to review)

jridgewell commented 2 years ago

Also ping @sarahghp and @chicoxyzzy

sarahghp commented 2 years ago

Likewise, this looks good to me, although I think I have gained more in learning from this project that I have to offer in corner-case excavation. (I did really enjoy taking the time to fully understand #4.)

Regarding the stage 3 review questions: I'm not certain this repository contains the expression of the need this is solving (as is mentioned in #1), but the slides do, so that should be an easy fix.

I am also curious if users will be surprised that the shortest shorthand const { #x } = this does not work but that the private field needs to be reassigned (especially where this.#x does is correct). I think the reasoning is pretty intuitive and teachable, but I do wonder how many people will be surprised the first time.

Overall a nice, limited update. 🎉

jridgewell commented 2 years ago

I'm not certain this repository contains the expression of the need this is solving (as is mentioned in https://github.com/tc39/proposal-destructuring-private/issues/1), but the slides do, so that should be an easy fix.

@sarahghp: Does https://github.com/tc39/proposal-destructuring-private#motivation not meet that? It's essentially a rephrasing of my original slides.

sarahghp commented 2 years ago

@jridgewell Yes, it does, I don't know how I missed that. 😳

waldemarhorwat commented 2 years ago

Looks good to me for stage 3.

chicoxyzzy commented 2 years ago

LGTM

littledan commented 2 years ago

Sorry for coming in here at the last minute, but... I think this proposal has significant implications for what private fields in object literals (as an expression) might mean. IMHO it would be great to eventually support that feature too, if so, its semantics should be consistent with this feature. Has this been investigated?

littledan commented 2 years ago

Concretely, this proposal takes the position that object literals do not create their own private scope, unlike class bodies. Personally I support this choice. See this investigation of multiple alternatives. I would prefer to move forward with private name declarations more generally (which Justin has proposed a version of), enabling both this feature as well as "package private" declaracions and strong encapsulation of objects, rather than only this piece. Or, if we only add a subset of that broader functionality, we should have a clear rationale for selecting that subset.

cole67 commented 8 months ago
  • [x] Problem statement #1
  • [x] Spec Text
  • [x] Reviewers

    • [x] @bakkot
    • [x] @rricard
    • [x] @waldemarhorwat
    • [x] @sarahghp
    • [x] @ljharb
    • [x] @chicoxyzzy
  • [ ] Editors

    • [x] @bakkot
    • [x] @syg
    • [x] @michaelficarra
cole67 commented 8 months ago

https://github.com/tc39/proposal-destructuring-private/tree/main