Open mbrowne opened 5 years ago
@igmat (me):
brand-checking
shouldn't be added implicitly to all private reads/writes, since it breaks metaprogramming with proxies as described in #106 and #134 Symbol.private
+ some kind of shorthand syntax for making symbols more ergonomic, provides much cleaner mechanism for encapsulationAfter talks with @ljharb, I concede that "undetectability" is a reasonable desire. Whether that reaches the level of a requirement depends on whether or not achieving it causes some existing functionality to fail or work in unexpected ways.
I don't remember wanting the ability to dynamically add a private field. I feel that would be counter-productive and dangerous.
I've also been working with the advocates of the classes-1.1 proposal on issues of functionality and implementation. You can count me as being in favor of that proposal.
Sorry @rdking, that must have been a bad recollection on my part. I removed that from the original post.
I don't remember wanting the ability to dynamically add a private field.
Someone want it. But I can't find the comment, too many 🤣
@hax I remember that too, from when I debuted the idea behind proposal-object-members. Someone thought that would enable the possibility for dynamically adding private fields (and indeed it could), but I said that wasn't part of the plan.
@mbrowne re: "dynamic private properties", I assume you're thinking of @shannon here.
I don't mind so much that they can't be created dynamically after construction. I just think dynamic access is a staple of JS and not having it is a huge oversight. Right now private symbols seems the best approach to achieve this in my opinion.
Summary of my feelings of this proposal
#
being part of the name leads to confusion because this.#x !== this['#x']
, it's just too weird. The sigil is fine but the functionality is just too different.@mbrowne Could you please add @shannon and @Igmat's feedback to the issue description? Just to avoid loosing them in thousand of comments like in the other issues.
@nicolo-ribaudo Sure - updated. I also added @allenwb's feedback.
@rdking I am trying not to add too many bullet points of closely related items or second tier concerns, but this one that you added seems particularly significant:
strongly believes that the best solution will integrate with existing features instead of crippling or disabling them.
Can you expand on that slightly, while still keeping it a concise summary? I'd like to add it to the issue description but it doesn't say what you're specifically referring to.
Or if you prefer that I completely replace your summary, you can post something for me to copy and paste.
CC @zenparsing @BrendanEich
@mbrowne The things I listed were in addition to what you've already got.
strongly believes that the best solution will integrate with existing features instead of crippling or disabling them.
Where this is concerned, I mean that, using the concept of a "private property", since this is just another property of an object, it is reasonable for the uninitialized to assume that access capability will be no different for public than it is for private, especially since they both take the same form. So with the existing feature being
obj.<property>
or obj['property']
the existence of a private property should not break this expectation.This is even more true in the case of Proxy where the completely undetectable private properties are not being stored in a separate object that uses the identity of the owner as a lookup reference, but somehow, Proxy still throws when dealing with a method accessing private fields. While the reason it throws is directly analogous to WeakMaps (the Proxy object itself is passed as the "target" to the handler functions), there's no reason for this to be the case, especially considering that developers implementing private fields in their libraries will be flooded with feedback from their users about use cases where whatever they used a Proxy for is now breaking merely due to the presence of the private fields.
Put shortly: If the proper introduction of a new feature into existing, working code bases that don't rely on the improper use of properties not intended for external use, causes the code to break, then the new feature has broken an existing feature.
Since you asked...
I believe that we are overpricing the benefit of this entire feature set while underpricing the risk.
Why do I think that we're overpricing the benefit?
private x
, perhaps with some optimization work applied to WeakMap on the engine side.Why do I think that we're underpricing the risk?
private x
from TypeScript) won't work. It doesn't "just work". We are taking on the risk that this feature will continue to create head-scratching across our community.#
for something else and it won't be available.Thanks for asking me to share my thoughts (I'm not particularly interested in debating though).
@mbrowne Thanks for the summary! This helps a lot.
Summary of my comment/issues:
Concept "Fields" is unclear and its necessity as an independent concept is questionable. The concept undermines the "Object is collection of properties" core concept, and the impact will be extremely far-reaching, but nobody taken seriously. The similar sound includes: Not Fields in Classes 1.1 proposal.
Using the lexical environment to implement "private field" is to recreate var
and ctx's VariableEnvironment, which is an abuse of Environment components, and it is obviously too bulky.
The syntax is very unfriendly, especially with #
as a prefix character for declaring fields and for name operations (such as the semantics of this["#x"]
vs. this.#x
). About grammar, I recommend enabling the private
keyword to declare private properties (Note: that I don't support new concepts like "Fields or Private Fields"). And #
as an operator, this is reasonable in both this#data
and #data
as unary or binary operators.
Reference: https://github.com/tc39/proposal-class-fields/issues/148 , https://github.com/tc39/proposal-class-fields/issues/100#issuecomment-429533532
Updated. @zenparsing and @BrendanEich, I mentioned you not to encourage you to join the debate (unless you want to), but just to give you an opportunity to have your feedback included in the issue description. Thank you everyone for summarizing your feedback.
I will summarize my own feedback as well:
[[Define]]
over [[Set]]
for initializers is a bad trade-off that throws away six years of evidence from TypeScript and Babel developers' use of fields. The "natural experiment" has been performed, the results are clear, and the runtime breakage incurred here is very difficult to detect with tooling. The proposed upside of [[Define]]
(const fields later) is purely speculative; the downside of not being able to install a deprecation setter is concrete and immediate.#
sigil is something we should just ignore and take the hit for, if built-in hard privacy is really needed. People will adapt and the runtime need for it is extremely clear; all counterproposals have been ignorant of the design constraints or runtime semantics.@RyanCavanaugh
Making initializers on fields optional, especially combined with the above, is unintuitive and dangerous. This syntactic expansion isn't strictly necessary and represents a substantial breaking change problem for TypeScript/Flow/Babel with no clear upside.
How is it a breaking change or problem for Babel? Or do you just mean when using Babel and Flow together? I think undefined
is a good default and don't see the advantage of explicitly initializing propertyName = undefined
considering that this would be very common.
@mbrowne Link
class Base {
prop = 10;
}
class Derived extends Base {
prop;
}
// Prints 10 in Babel 6.26, would print 'undefined'
console.log(new Derived().prop);
This would be so much simpler and avoid many of the issues raised if redeclaring the same property in a subclass were simply illegal...you can already set different initial values if you want (or even call Object.defineProperty
) in the constructor.
@mbrowne Since properties are either data properties or accessor properties, it might be better if redeclaration of a data property that doesn't changing the property type from data to accessor is illegal. Likewise, changing from accessor to data should also be illegal. Redefining accessor properties as new accessor properties should be allowed as this might represent some useful change to the accessors.
@RyanCavanaugh
The proposed upside of [[Define]] (const fields later) is purely speculative;
...and irrelevant since even with [[Set]], the action can be immediately followed by a change to the writability of the property, leaving all side effects of setting the value in tact.
If possible, can I suggest we keep discussion of specific points to other threads? (Anything beyond clarifications, anyway.) I'd like to keep this thread as an accessible summary.
@RyanCavanaugh
Honestly, the negative response to the # sigil is something we should just ignore and take the hit for, if built-in hard privacy is really needed. People will adapt and the runtime need for it is extremely clear; all counterproposals have been ignorant of the design constraints or runtime semantics.
- Kinda difficult to be aware of design constraints that are never revealed.
- The runtime semantics of the current proposal is crippling enough to break a sizable portion of the code in the wild should library authors decide to rewrite making use of this proposal's version of private fields.
- While the counterproposals do take into account different constraints, the general goal of adding private in such a way that if the only changes made are to make private those properties meant to be implementation details, then no code that did not make ill-advised use of such fields will be broken, is the one thing they all have in common, while differing with the current proposal.
- ES does not need built-in hard privacy. It's just an extremely strong want. Hard privacy is already well provided for via judicious manipulation of closures and WeakMap. I would much rather see the TC39 take this proposal back to stage 1 or 2 so it can be re-formulated than see it implemented, and watch as people are frustrated by unnecessary code-bloat (due to lack of
[]
support for private fields), broken libraries (due to incompatibility between Proxy and private fields), disabled functionality (due to field definition being a [[Define]] instead of a [[Set]] causing prototype properties to be ignored unexpectedly), and other such issues.
@RyanCavanaugh
- Honestly, the negative response to the
#
sigil is something we should just ignore and take the hit for, if built-in hard privacy is really needed. People will adapt and the runtime need for it is extremely clear; all counterproposals have been ignorant of the design constraints or runtime semantics.
Even hard privacy is must, both classes 1.1 and Symbol.private proposal meet this requirement. Basically instance variable in classes 1.1 keep the same semantic of current private field, but use different syntax which avoid controversial #
. The other big difference between classes 1.1 with current proposal is classes 1.1 doesn't include public field, but it seems you agree current semantic of public field has many problems.
Please, let's keep this thread topical and avoid back and forth technical discussion. The purpose was to summarize points of view to be accessible to people who don't have time to read very long threads.
@littledan could you please clarify how this summary will affect proposal and / or process?
If there's no legible summary, the whole discourse in the repository is likely to be ignored by the vast majority of observers, who have no time to read through all the back and forth.
I can't say that the summary will cause a change in the outcome because, as I've explained, TC39 has repeatedly reaffirmed consensus on the proposal in this repository, and implementations are moving ahead with it.
@littledan As a detractor of the current proposal I have to wonder what would it take to make TC39 willing to revert the current proposal to an earlier stage and at least allow competing proposals to enter stage 0 or 1?
How long will the community have to try out the implementations and provide feedback before the committee makes any final changes and moves it to stage 4?
@rdking I doubt that more detailed explanations of your rationale are the limiting factor here. I don't have good advice for you as to how to get the committee to reconsider, given that 1) Personally, I hope we all go ahead with this proposal, so I'm not the best person to ask 2) There were three detailed efforts to get the committee to reconsider in various different ways, and consensus on this proposal was reaffirmed.
@mbrowne I hope that, in general, this sort of developer feedback can be taken into account in considering proposals for whether to promote to Stage 3. In the future, I'd like to make sure we have a setup so people can try out most proposals before Stage 3, but TC39 has decided to not make that a requirement in the stage process in general.
In this case, we didn't have transpiler or browser implementations until a bit after Stage 3 (one was started in 2016, but it took a while to finish it up and land it). Now, the feature has been available in both Babel 7 (including pre-releases) and Chrome behind a flag for several months. I haven't heard any feedback over the past several months which was not discussed in TC39 earlier, so I feel like we've gotten a pretty full sense of the opinion landscape.
At this point, given the Stage 3 status and the long period for feedback and discussion, some browsers are likely to ship the proposal as is (e.g., Chrome did recently turn on public fields in Canary), not waiting for Stage 4.
@mbrowne implementations of class fields are available now.
Google shipped a flagged implementation in V8 6.6 back in March 2018. In April 2018 this was released in both Chrome 66 (stable) and Node 10.0. Try it in Chrome by enabling chrome://flags/#enable-javascript-harmony or try it in Node using --harmony_class_fields
.
If you want to run something in other browsers, full support for class fields arrived in Babel 7 which was released in August 2018.
@littledan this is incredibly disheartening. The feedback over the last several months in the GitHub repo may not be new idea but the sheer amount if it should convince you that something is wrong. Every time the answer has been we will make 0 changes and we are moving ahead.
As someone who has tried the various inplementations, it hasn't gotten any eaiser in practice. I find my self frustrated with the constraints and I simply don't want to use it. My problem as a library developer is I have to plan for consumers to use it so avoiding it is impossible. It has not been easy planning. The fact that class decorators removed access to private names threw a huge wrench in this planning and I had to scrap a ton of code. This approach was suggested to me but TC39 members here so it's even more frustrating.
This proposal in no way make my job eaiser. It will only make it harder. The entire process has kept me up at night and I find myself wanting to just walk away from it and not contribute to the conversations anymore. Unfortunately JavaScript, a language I very much love, is the I only natively supported language. There is no alternative. Choosing another language to avoid the decisions here is simply not feasible.
If there is no way to avoid the issues described here, and they have all been reasoned about, and there has always been a consensus that this is the only way forward, well then the only option is to not move forward at all. Moving forward will be good for a few but very bad for many.
@shannon Have you written down how this proposal affects your project in particular? If so, I missed that.
The feedback over the last several months in the GitHub repo may not be new idea but the sheer amount if it should convince you that something is wrong.
That "something" is mainly the syntax not looking like Java, and it's well-covered ground.
Moving forward will be good for a few but very bad for many.
This has not been established.
@littledan yes in #75. In particular @bakkot recommended that I use a class decorator for iteration. This is not possible anymore. I can handle it other ways if dynamic access is provided but without it it's basically impossible (or at least incredibly complex that I don't even want to attempt it) and I have to give up my plan of an API that consists of mainly decorators. A nice simple declarative API. So now decorators become less useful to me, proxies become less useful to me (#106), and classes as a whole become less useful to me. I'm not sure where else to go with this API other than to just start over form scratch and write a functional API which I really, really don't want to do.
@bakkot's comments that so many private fields would be uncommon just baffles me. Developers are going to switch to private by default and there will be mostly private fields. In application land this will make no difference. In library land it will be a headache and a burden.
@slikts
That "something" is mainly the syntax not looking like Java, and it's well-covered ground.
Not helpful. It's in no way mainly about the syntax as several people have described in great detail. I don't particularly feel like debating this again so I'm going to stop here.
@littledan I can understand why the committee is not interested in continuing to reconsider the syntax and probably has the feeling that it has been discussed to death. But some of the issues being raised are not about syntax and aren't even about private fields, such as #151. I fear it may do irreparable damage to the relationship between the committee and the community if this proposal moves ahead without making a strong technical argument for disagreeing with so much community feedback and sticking to the current proposal 100%. I think the committee has already presented strong arguments in favor of the private field syntax (if not the full reasoning behind the requirements), but that is not the only issue people are concerned about.
To me the most concerning issue at the moment is using [[Define]] semantics instead of [[Set]]. I hope the committee's decision on this will turn out to be for the best, and I and others would really like to understand the reasoning better. As of now, half of the argument presented in favor of [[Define]] has been that developers have already been using it without issues, allegedly proving that it works just fine in the wild. But as many people have pointed out, most of the people trying out public fields via Babel weren't doing so in spec mode, so if Babel usage is what @bakkot was referring to then I don't find that a convincing argument. And the other half of the justification that @bakkot pointed to seems fairly weak in comparison with the insidious foot-gun issues we have been discussing in recent days.
As you suggested, let's discuss the details of these arguments in the other threads...the point I want to make here is that it's important to show that the community feedback has been considered on all the issues, not just the most discussed ones like the private field syntax. It seems that hardly anyone here who's not on the committee fully understands and agrees with the decision to use [[Define]] (via [[CreateDataPropertyOrThrow]]). I'm sure the committee made their decision very carefully; please help the rest of us to understand it.
Also, given that the implementations weren't available until later in the process than other proposals, I think it would only be fair to allow more time than usual before advancing this proposal to stage 4, especially with such a controversial proposal. I really hope the Chrome team does not decide to include fields (even public fields) without a flag in an official version of Chrome before it is 100% standard. I seem to be one of the few community voices here who thinks this is a solid proposal and I look forward to the availability of the class fields feature, regardless of the decision on [[Set]] vs. [[Define]]. But please remain open to making some final changes if community feedback is able to empirically show that they would be an improvement.
@slikts
This has not been established.
That's because no attempt has been made to establish this fact. In the end, this proposal will ultimately spoil what little advantage there is to be gained from using class
for a great many developers. Something as important as this should be thoroughly tested along side competing ideas to see which fairs best in the wild, not decided by a limited group of (highly skilled) experts with an unfortunate case of jading due to how long this feature has been in discussion.
It's in no way mainly about the syntax as several people have described in great detail.
Most of the "sheer amount" of feedback is indeed about the syntax.
… unfortunate case of jading due to how long this feature has been in discussion.
That's quite uncharitable towards the members.
Edit: I didn't like this version so I rewrote it....
It's in no way mainly about the syntax as several people have described in great detail.
Most of the "sheer amount" of feedback is indeed about the syntax.
You misunderstand. The sheer amount of feedback is indeed loaded with syntax issues, but that's only because syntax partially prescribes functionality. We want functionality that is consistent with the existing language. What that looks like, I'm sure most of us don't really care that much so long as it's intuitive and resembles something you can already do. The problem is, no matter how that functionality plays out, the syntax needs to be able to almost intuitively describe to the developer what is happening. This is why the provisions of the current proposal requires explanation. The syntax used does not intuitively describe the functionality. So it winds up getting attacked along with the undesirable functionality.
… unfortunate case of jading due to how long this feature has been in discussion.
That's quite uncharitable towards the members.
It's a matter of perspective. I'm quite thankful that TC39 has put so much effort into trying to land a solid private data proposal. However, given that they've been at this for over a decade, but somehow
and on top of all that, have refused to give other viable proposals an opportunity to compete in the community, all the while trying to land this proposal, even with all of its issues that even some members of TC39 itself want to avoid, I think the word "jaded" fits this condition well without being an insult. To all who feel insulted by this term, I am sorry you feel that way. However, I don't know a more fitting term for what appears to be the condition.
I don't know if I've voiced it but my main objection to the current private fields proposal (and more so private methods) is that it throws away the notion of prototypal inheritance in favour of an object branding mechanism inconsistent with how other properties work.
I think my other objections could be resolved with additional proposals so I won't list them here.
@Jamesernator It would help the advocates of this proposal understand the issues with proposal class-fields if you'd go ahead and list them. We'd all be interested.
IMO this proposal should include reserved space (syntax, patterns of usage mirroring private, etc) for protected members, so that if it is later decided that protected will be added to the spec that it will be possible to do it without protected having different patterns and mechanics than private aside from access visibility (f.e. syntax patterns and patterns of use should be the same, if only varying by a sigil). It seems only private members are given the limelight in this proposal.
The word "protected" is not mentioned even once in the spec. There's many people who want protected, and it makes sense that if it might be added later, it should be able to shares similarities with private.
objection to the current private fields proposal (and more so private methods) is that it throws away the notion of prototypal inheritance
and it throws away the core strength of JS: dynamicism.
It would be great not to throw these away. In my implementation (lowclass
),
Protected
and Private
helpers are references which can be purposefully leaked in order to create concepts such as "package protected" (using the "gateway pattern" to force a critical error to be thrown when a non-friend module imports a dependency), "friends", "module protected" (protected or private features shared only among classes in the same module or closure), etc.
It'd be nice for the native implementation to be able to match the dynamic nature of JavaScript.
If the new features comes out but I can't do the same as I can with lowclass, I may as well just not use the new features if I can achieve more with my own.
I like this explanation of why protected fields should be avoided.
Hi all, This is intended as a summary thread, and we are trying to keep it short so that people can come here and get a quick overview of community feedback.
@trusktr In the interest of helping you find more info and other threads to post in if you have more to say, first of all I think it's worth mentioning that a lot of past discussions have covered some important considerations that are different for native features than libraries on top of the existing language. Secondly, regarding protected
, see for example https://github.com/tc39/proposal-class-fields/issues/144#issuecomment-430585551 and https://github.com/rdking/proposal-class-members/issues/3#issuecomment-436042432.
@slikts This is one of the rare occasions when we agree. protected
is a dangerous tool, much like a butcher knife. It's easy to cut yourself if you're not careful. However, it's still a necessary tool. As was said in that post:
But as you see, all of these are 'tend to'. Sometimes a protected member is the most elegant solution. And protected functions tend to have fewer of these issues.
Just because a tool is dangerous doesn't mean it shouldn't exist at all. Sometimes it's truly the best way to handle the situation.
Continuing from #100 and other threads, here's a summary of key objections to this proposal raised by the community:
@hax's top 3 concerns:
Also, he believes another long wait for private state is acceptable if that's what it takes to get it right: the problems with the current proposal—including with public fields—are just too severe.
Regarding the substance of the proposal:
#
syntax does not fit into the mental model of JS devs and will cause a community split, and TypeScript issues will further fracture the community@rdking:
obj['property']
for public fields but no equivalent for private fields breaks existing feature by breaking expectations. Lack of dynamic access of private fields (e.g.this['#x']
) is a deal-breaker.#
is not "well-defined"—has two meanings. Context is insufficient to disambiguate declaration and access.@bdistin:
@mbrowne (me):
(NB: it's mbrowne, not mbrown)
private #x
syntax should be strongly considered because#
alone might make future native support of intermediate access levels impossible. But#
alone might be OK if we are confident we could have another way of natively sharing private fields within a module.@Igmat:
brand-checking
shouldn't be added implicitly to all private reads/writes, since it breaks metaprogramming with proxies as described in #106 and #134Symbol.private
+ some kind of shorthand syntax for making symbols more ergonomic, provides much cleaner mechanism for encapsulation@shannon:
#
being part of the name leads to confusion becausethis.#x !== this['#x']
, it's just too weird. The sigil is fine but the functionality is just too different.@aimingoo:
var
and ctx's VariableEnvironment, which is an abuse of Environment components, and it is obviously too bulky.this["#x"]
vs.this.#x
). About grammar, I recommend enabling theprivate
keyword to declare private properties (Note: that I don't support new concepts like "Fields or Private Fields"). And # as an operator, this is reasonable in boththis#data
and#data
as unary or binary operators.Reference: #148 , #100 (comment)
@RyanCavanaugh:
[[Define]]
over[[Set]]
for initializers is a bad trade-off that throws away six years of evidence from TypeScript and Babel developers' use of fields. The "natural experiment" has been performed, the results are clear, and the runtime breakage incurred here is very difficult to detect with tooling. The proposed upside of[[Define]]
(const fields later) is purely speculative; the downside of not being able to install a deprecation setter is concrete and immediate.#
sigil is something we should just ignore and take the hit for, if built-in hard privacy is really needed. People will adapt and the runtime need for it is extremely clear; all counterproposals have been ignorant of the design constraints or runtime semantics.@jamesernator
In addition, some members of the TC39 committee also have current objections to this proposal. For example:
@allenwb:
Design Rules: Item 2:
Item 3:
Item 4:
@zenparsing:
private x
from TypeScript) won't work. It doesn't "just work". We are taking on the risk that this feature will continue to create head-scratching across our community.#
for something else and it won't be available.I probably missed something important, and obviously I haven't included a summary for everyone who has commented. If you feel something is missing, please provide a concise summary of your feedback as your first comment on this thread (saving longer arguments for subsequent comments). Also feel free to simply say, "I agree with [username]".