tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

A summary of feedback regarding the # sigil prefix #100

Closed glen-84 closed 6 years ago

glen-84 commented 6 years ago
Issue/comment šŸ‘ šŸ‘Ž šŸŽ‰ ā¤ļø
Stop this proposal 196 31 20 45
Why not use the "private" keyword, like Java or C#? 132 7 12 5
Why not use the "private" keyword, like Java or C#? šŸ’¬ 144 0 16 9
Why not use the "private" keyword, like Java or C#? šŸ’¬ 51 0 0 0
Private Properties Syntax 33 4 0 0
Yet another approach to a more JS-like syntax 32 0 0 41
New, more JS-like syntax 32 4 0 0
Please do not use "#" 32 0 1 1
Proposal: keyword to replace `#` sigil 18 0 0 1
Why not use private keyword instead of #? 16 0 0 1
Explore a more JS-like syntax 7 0 0 0
The '#' looks awful, please change it 9 4 0 0
can we just use private keyword? 2 0 0 0
Do we still have a chance to stop private fields (with #)? 3 3 1 1
[Private] yet another alternative to `#`-based syntax 3 0 0 0
and yet another suggestion for "private but not closure" 0 0 0 0
Why don't use `private` instead of `#`? 0 3 0 0
Why "private" and "protected" future reserved keywords are not used? 0 0 0 0

Other feedback:

The TypeScript team are not fond of the syntax either.

My suggestion to rename the proposal: https://github.com/tc39/proposal-private-fields/issues/72 (the idea is mainly about making it a lower-level feature, and making room for a cleaner syntax at a later date)

@littledan @bakkot @ljharb ā€“ I apologize in advance, as I know that you must be thinking "Oh g*d, not again!", but I just wanted to put this all in one place, and allow the community to share their opinion in the form of reactions. It would be great if you could keep this open for a while (30 days?), or perhaps even indefinitely to prevent other users from submitting even more issues on the same topic.

To the non-committee members reading this: These proposals are now at stage 3, so it's unlikely that a syntax change will occur, but please feel free to up-vote this issue if you feel that the sigil (#) prefix is not a good fit for the language, or down-vote the issue if you believe that it's perfectly acceptable.

rdking commented 6 years ago

@shannon Finally made it through that thread. In the end, your idea(#75) and mine(#104) share a lot in common. There are a couple of key differences that I would like to have considered:

  1. Move the sigil to behind the object as a postfix, binary operator.
  2. Because of point 1, the left-hand parameter for this operator is always an object, while the right is always an access operator (. or []). The point is to make sure it is well understood that while the operator can be used to access members of the private internal object, it cannot provide the object itself. This operator should be the only means of accessing the private space.
  3. Because of point 2, destructuring and iteration with for ... in and for ... of loops is out.
  4. Because of point 2, Object.someFn() can't be used to glean info about the private fields. That shouldn't be an issue since all the keys are known at time of declaration, they always exist, and they cannot be deleted.

In this way, we avoid some risks:

class Test {
   #foo = 1;
   #bar = 2;
   #foobar = 4;
   #fubar = 8;

   getPrivateData() {
      return #this; //Shouldn't be allowed
   }
};

One of the key requirements for this proposal was that there is no way to directly expose the private data.

rdking commented 6 years ago

Another problem to be avoided is nesting. What if an instance contained another instance?

class Test {
   #other = null;
   #value = 0;
   constructor(other) {
      #this.other = other;
   }
   changeValue(newVal) {
      #this.value = newVal;
      //How do I change value on other?
      #(#this.other).value = newVal; //should work, but cumbersome
      ##this.other.value = newVal; //does this even make sense?
      #this.#other.value = newVal; //does this work or syntax error?
      //What if we needed to get silly with it?
      #(#(#this.other).other).value = newVal; //??
   }
}

With the suggestions I'm offering, it'd look like this:

class Test {
   private other = null;
   private value = 0;
   constructor(other) {
      this#.other = other;
   }
   changeValue(newVal) {
      this#.value = newVal;
      //How do I change value on other?
      this#.other#.value = newVal;
      //What if we needed to get silly with it?
      this#.other#.other#.value = newVal;
   }
}

Making the sigil a postfix binary operator in this way makes it very clear how its supposed to be used and keeps the syntax clean.

shannon commented 6 years ago

@rdking I agree that moving it to a postfix operator makes more sense in terms of nesting. This concern was brought up in the other thread and I didn't have a good solution.

Variable access is a start to some of the concerns I had.

For ... of may be out but at least you can do something like this:

for (const value of ['x', 'y', 'z'].map(key => this#[key])) {

}

Which is starting to look a bit better for what I was working on.

Anyways this looks good to me. I look forward to seeing the responses from the others.

glen-84 commented 6 years ago

JavaScript Classes 1.1 isn't perfect, but the syntax is a lot better than this proposal. I really hope that it is considered seriously before this proposal moves to stage 4.

ljharb commented 6 years ago

@glen-84 it was considered but does not meet use cases the committee has long since committed to solving, and failed to achieve consensus.

The current proposal - public and private, static and instance - is stage 3. Itā€™s not ā€œtoo lateā€ to change things if alternatives that meet the criteria are offered, but theyā€™d also have to be significantly compelling to warrant the effort.

PinkaminaDianePie commented 6 years ago

I hope that class fields will be accepted without a private modifier. So many other proposals were dropped or postponed before, just because they were not so clear or received some negative feedback. The worst thing here is backward compatibility - if the bad solution will be accepted, after some time it will be impossible to replace it with something better or remove entirely. We have some "bad parts" in JS and need to deal with them, just because of historical reasons. Please don't add another one. People's feedbacks are totally different about public class fields and about private modifier. Why not split proposal on 2, accept public fields and return the private part for the redesign? We will still get a useful feature, but without controversial parts, which can be improved and added later.

glen-84 commented 6 years ago

@glen-84 it was considered but does not meet use cases the committee has long since committed to solving, and failed to achieve consensus.

It was rejected already!? šŸ˜®

...

What if this proposal used some of the syntax from that proposal?

class Example {
    public a = 1; // The public keyword is optional (it's the default)
    public b = 2;

    hidden a = 1;
    hidden b = 2;

    constructor(a, b) {
        this->a = a;
        this->b = b;
    }

    public example() { // The public keyword is optional (it's the default)
        // ...
    }

    hidden example() {
        // ...
    }
}

Why?

If this is even semi-viable, I can create a new issue.

bakkot commented 6 years ago

@glen-84 Yes, we discussed it in committee. The committee was not enthused. Similar syntax has been discussed before, for example here, and I don't think it needs to be rehashed yet again.

(Incidentally, the committee has in the past been very strongly opposed to an optional public modifier, feeling it ought to be mandatory or forbidden.)

zenparsing commented 6 years ago

@PinkaminaDianePie

Why not split proposal on 2, accept public fields and return the private part for the redesign? We will still get a useful feature, but without controversial parts, which can be improved and added later.

I agree 100%. This is absolutely the low-risk approach that we should take.

@glen-84

Love it! @littledan @bakkot @ljharb we should discuss @glen-84 's suggestion further.

ljharb commented 6 years ago

I donā€™t see the point. This suggestion doesnā€™t have the same syntax for declaring and usage, for one.

This is a stage 3 feature. Itā€™s been split before, and is now re-merged. I donā€™t find the alternative nearly compelling enough to warrant changing anything at this point. You say ā€œlow-riskā€, but thereā€™s not ā€œriskā€ from proceeding as-is - only a vocal minority with an aesthetic dislike.

bakkot commented 6 years ago

The reason for keeping the two joined is that their designs are closely related - for example, @glen-84's proposal involves a change to public fields, your 1.1 proposal involved leaving out public fields, etc. As such I feel strongly that they should remain merged, whether or not we revisit the syntax here.

Though also I don't think we should revisit the syntax here. We have discussed many, many syntax alternatives, including this most recent one, and always settled on the current design. I don't see a particular reason to think that's going to change.

glen-84 commented 6 years ago

@glen-84 Yes, we discussed it in committee. The committee was not enthused. Similar syntax has been discussed before, for example here, and I don't think it needs to be rehashed yet again.

Wow ā€“ to address my own comment (things change ...):

I'm -1 on -> ...

    1. It's used by C++ and PHP for other reasons
    2. It would look very strange on its own (console.log(->prop))
    3. It could mess with CoffeeScript as well (?) (I don't use it, just wondering)
  1. Well, there are few options as this point, and the committee (at least part of it) doesn't seem open to delaying/splitting private fields
  2. Can require this
  3. I think CoffeeScript is dead, but I could be wrong.

(Incidentally, the committee has in the past been very strongly opposed to an optional public modifier, feeling it ought to be mandatory or forbidden.)

Then make it mandatory if you must. You only have to type pu and IntelliSense kicks in. Then it'll get compressed away.

ljharb commented 6 years ago

In JavaScript, public is the default. I would be opposed to requiring that tax on the majority case.

glen-84 commented 6 years ago

I donā€™t see the point. This suggestion doesnā€™t have the same syntax for declaring and usage, for one.

Neither does public access? And AFAIK this isn't a technical requirement anyway.

You reason about it like this: . refers to properties, -> refers to hidden members (including methods)

I donā€™t find the alternative nearly compelling enough ...

Do you honestly think that the # is more readable than the above suggestion?

only a vocal minority with an aesthetic dislike

Minority is your opinion, there's no proof of that. Look at the comments on the 2ality blog post. Is it the minority speaking in the top 3 comments?

It's not only about aesthetics, but also readability and fitting in with the language.

bakkot commented 6 years ago

Do you honestly think that the # is more readable than the above suggestion?

Personally, yes. The world really does not need another overload for -> access. And the symmetry of class { x = 0; #y = 1; m(){ return this.x + this.#y; } } is compelling to me.

glen-84 commented 6 years ago

We have discussed many, many syntax alternatives, including this most recent one, and always settled on the current design.

How many of the "many, many" were actually viable? I'm genuinely interested in the other technically viable options. I wish that they were listed somewhere.

glen-84 commented 6 years ago

In JavaScript, public is the default. I would be opposed to requiring that tax on the majority case.

That's why I suggested making it optional. In a perfect world it's best to avoid choices, but in this case it would allow users to keep a symmetry and be explicit.

bakkot commented 6 years ago

Depends on what "technically viable" means. A majority of alternatives discussed managed to be feasibly parseable and not breaking, I think. The debate has always been over the ergonomics of and the mental model implied by the syntax.

glen-84 commented 6 years ago

I noticed that @allenwb was also involved in the js-classes-1.1 proposal. May I ask how many of the committee members are in favour of the hash sigil (actually think it fits, not just "well I guess we have no option"), and how many would prefer an alternative syntax?

PinkaminaDianePie commented 6 years ago

That's why I suggested making it optional. In a perfect world it's best to avoid choices, but in this case, it would allow users to keep a symmetry and be explicit.

Actually, it's even worse than the current proposal. As for the current proposal, I can just add a rule to the eslint, which will forbid using private fields, and I will have just the same capabilities as I had before, but with your proposal, I will get nothing at all but will need to write more code without any benefits.

littledan commented 6 years ago

I agree with @bakkot that we've considered this alternative and identified problems with it previously. Thanks for linking to related resources, @bakkot. If anything about the reasoning there seems unclear or insufficient, let's discuss that here.

I don't see a particular reason that thoughts about this proposal should lead to a delay or splitting off of other features. If anything, the fact that "new" proposals keep being more or less reiterations of previous proposals that we've thought a lot about is evidence that we've done our homework here and are on the right track.

glen-84 commented 6 years ago

A majority of alternatives discussed managed to be feasibly parseable and not breaking, I think.

It never seemed that way, after reading many of those discussions. It almost sounded as if it was "impossible".

The debate has always been over the ergonomics of and the mental model implied by the syntax

Somewhat subjective. And it's the committee members that make this decision, based on their idea of what is ergonomic?

zenparsing commented 6 years ago

@littledan @ljharb @bakkot

There has been a consistent drumbeat of disregard for user feedback here that I think is very problematic. We're going to need to discuss this further.

glen-84 commented 6 years ago

Actually, it's even worse than the current proposal. As for the current proposal, I can just add a rule to the eslint, which will forbid using private fields, and I will have just the same capabilities as I had before, but with your proposal, I will get nothing at all but will need to write more code without any benefits.

ljharb commented 6 years ago

@zenparsing given the constraints, i do not think the feedback weā€™ve gotten makes ā€œno private everā€ worse than ā€œthe current proposalā€. We can of course keep discussing it, but i feel pretty confident that there will be no consensus on anything but the current proposal at its current stage.

@glen-84 i block for..of and generators and async/await with linting rules; it doesnā€™t say nearly as much as youā€™d think.

glen-84 commented 6 years ago

I agree with @bakkot that we've considered this alternative and identified problems with it previously. Thanks for linking to related resources, @bakkot. If anything about the reasoning there seems unclear or insufficient, let's discuss that here.

Where can I find that reasoning (without re-reading thousands of posts)? I don't think that there is anything in the (well-written) FAQ about this specific combination.

I don't see a particular reason that thoughts about this proposal should lead to a delay or splitting off of other features.

Has anyone questioned, or opened a single issue regarding public fields or static declarations? It's a no-brainer AFAIK. That's why splitting should be considered, IMO.

If anything, the fact that "new" proposals keep being more or less reiterations of previous proposals that we've thought a lot about is evidence that we've done our homework here.

Or that fact means that there is more agreement there? It was not my intention to "copy" the work of Kevin and Allen, I just thought that "merging" the best from both proposals might lead you to a more pleasing solution.

glen-84 commented 6 years ago

@glen-84 i block for..of and generators and async/await with linting rules; it doesnā€™t say nearly as much as youā€™d think.

Blocking async/await, how dare you! šŸ˜†

PinkaminaDianePie commented 6 years ago

public is the default, what additional code would you have to write?

That's what I'm talking about, it was by default and should stay the same, and making it explicit is a bad idea.

zenparsing commented 6 years ago

@ljharb Let's be clear. The feedback, from day one (back when I created the initial specification) was that users, in the large, don't want a sigil for this. We do our users a disservice when we ignore this very strong signal and form "consensus" in the opposite direction (funny word, "consensus"!).

glen-84 commented 6 years ago

@PinkaminaDianePie It would stay the same ... adding it would be optional.

PinkaminaDianePie commented 6 years ago

It would stay the same ... adding it would be optional.

I probably didn't get your proposal, i thought that you meant that it's good to make "public" explicit in case if we'll have explicit "private" keyword.

ljharb commented 6 years ago

@zenparsing i understand that, but thereā€™s no way to meet all the criteria without a sigil - so the fact that users donā€™t want a sigil either has to lead to the feature never occurring, or the feedback being trumped by the need for the feature.

thysultan commented 6 years ago

You say ā€œlow-riskā€, but thereā€™s not ā€œriskā€ from proceeding as-is - only a vocal minority with an aesthetic dislike.

A vocal minority is a misrepresentation. The premise of this issue and the documented vocal backlash from more than just GitHub issues/comments referenced in https://github.com/thysultan/proposal-private-methods-and-fields#syntax-synergy is evident.

Further more it is not "just" the aesthetic dislike that screams a crippling advent of loosing syntax synergy, there are multiple issues that have been ignored.

The whole premise that .# is like ._ is flawed as _ is valid subscript access ["_"]. The this namespace is already a hive for overloaded confusion and the articles about this go by the dozens, adding fuel to the fire won't make that any better, especially with the dis-symmetry introduced.

Another aspect that escapes this proposal is the static/dynamic nature of definition, and the in-ability to assign to a private field at runtime.

The semantics for proposal-private-methods-and-fields avoid these issues.

but thereā€™s no way to meet all the criteria without a sigil.

It's not that there is no way, this is only based on the premise that the criteria(which is largely subjective) is the gold standard at-all, a point nicely articulated by @rdking in https://github.com/tc39/proposal-private-methods/issues/33

glen-84 commented 6 years ago

I probably didn't get your proposal, i thought that you meant that it's good to make "public" explicit in case if we'll have explicit "private" keyword.

You can do either:

class Example {
    public a = 1;
    public b = 2;

    hidden a = 1;
    hidden b = 2;

Or:

class Example {
    a = 1;
    b = 2;

    hidden a = 1;
    hidden b = 2;

I prefer the symmetry and explicitness of the first, but I wouldn't force everyone to use it.

FWIW, it's optional in TypeScript, and AFAIK, no one has died because of it. (related SO post)

If you are using TypeScript (it's amazing), then you could also have currently-soft private and protected as well:

class Example {
    public a = 1;
    public b = 2;

    protected a = 1;
    protected b = 2;

    private a = 1; // Soft private, for now at least.
    private b = 2;

    hidden a = 1;
    hidden b = 2;
doodadjs commented 6 years ago

@ljharb: We all know you are 99.99999% against classes. You might be also blocking the "class" keyword with ESLint. So, what I don't understand is your strong interest for the "hard private" class fields.

PinkaminaDianePie commented 6 years ago

So, what I don't understand is your strong interest for the "hard private" class fields.

It's a trick to make classes worse than they are and finally, add them to eslint rules :D (in that case I will like Airbnb guide even more TBH)

But really, is this issue about syntax only or about all concerns such as "hard private"? For me "hard private" is much worse than syntax itself, but i think it was discussed in some other issue.

bakkot commented 6 years ago

For me "hard private" is much worse than syntax itself, but i think it was discussed in some other issue.

Yes, that was discussed mainly in https://github.com/tc39/proposal-private-fields/issues/33.

ljharb commented 6 years ago

@doodadjs Iā€™m against inheritance; but i insist on using class for it. Either way, private fields are a critical need to have true encapsulation with classes - so letā€™s keep unrelated discussions from irc, on irc, thanks.

doodadjs commented 6 years ago

private fields are a critical need to have true encapsulation with classes

I hope that kind of encapsulation is inheritance-friendly. I don't see a lot of talks about "protected", most are granted to "public" and "private". Will "protected" be another proposal ?

EDIT: Last question moved to issue #105.

allenwb commented 6 years ago

@ljharb

@zenparsing i understand that, but thereā€™s no way to meet all the criteria without a sigil - so the fact that users donā€™t want a sigil either has to lead to the feature never occurring, or the feedback being trumped by the need for the feature.

This is only true that if you define the requirements (the criteria) such that only a sigil based design can satisfy them. The "classes 1.1" proposal and associated discussions show that there are alternative designs that could be followed if TC39 is willing to reexamine and simplify the requirements.

There is a 20 year trail of tears of failed ES class proposals that attempted to satisfy an over-constrained set of requirements.

doodadjs commented 6 years ago

(in that case I will like Airbnb guide even more TBH)

@PinkaminaDianePie What/Who tells you that classes are bad ? Airbnb ?

I have nothing against Airbnb, but that should stop to their good contributions. Thank you.

PinkaminaDianePie commented 6 years ago

What/Who tells you that classes are bad ? Airbnb ?

No, guys like Douglas Crockford or Dan Abramov. And a lot of others: https://github.com/joshburgess/not-awesome-es6-classes

doodadjs commented 6 years ago

No, guys like Douglas Crockford or Dan Abramov.

@PinkaminaDianePie hmmm... I don't know neither, but I assume they are very very very important every day developers...

And a lot of others: https://github.com/joshburgess/not-awesome-es6-classes

@PinkaminaDianePie I'll read that carefully, and come back, thanks.

PinkaminaDianePie commented 6 years ago

hmmm... I don't know neither, but I assume they are very very very important every day developers...

Every day developers use results of their work every day, so yes :)

Sorry for offtopic :(

doodadjs commented 6 years ago

Every day developers use results of their work every day, so yes :)

@PinkaminaDianePie Fine, same for me... Sorry for being off-topic.

ljharb commented 6 years ago

@allenwb TC39 is not willing to remove any of the requirements, and in general does not consider that "simplification", which is part of why 1.1 was unable to gain anything close to consensus.

shannon commented 6 years ago

@ljharb is there is a list of the TC39 requirements that others can view?

Perhaps it would help to add these requirements to the FAQ. I mean the FAQ has some logic and reasons for choices but it doesn't really differentiate between what is a hard requirement and what is just what the proposal developers think is the best approach.

littledan commented 6 years ago

@shannon The goals that the committee adopts for proposals is largely on a case by case basis, but we're working on documenting some of our design considerations and hope to have something published soon.

glen-84 commented 6 years ago

@zenparsing Was there any further discussion regarding my suggestion? Is it worth opening a separate issue, or would it be a waste of time?

zenparsing commented 6 years ago

@glen-84

Is it worth opening a separate issue

What do you think? Is your proposal something that you can see yourself using (and enjoy using)? For my part, I would be more than happy to continue discussing the pros and cons.

I would probably leave the "optional public" aside (or just as a note on integration with TypeScript), since it may distract from the main idea.