Skip to content
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

Use [[Set]] by default, add `define` keyword. (was: TypeScript class fields have [[Set]] semantics.) #248

Open
trusktr opened this issue Jun 17, 2019 · 36 comments

Comments

@trusktr
Copy link

commented Jun 17, 2019

Class-fields in TypeScript have [[Set]] semantics. Foresee code breaking when JavaScript projects convert to TypeScript (or vice versa), and the pain caused from hard-to-find bugs, because of JavaScript class-fields using [[Define]] semantics.

I know this has been discussed in other threads, but I thought I'd open an official issue for it.


EDIT: Why not add a define keyword:

class Foo {
  foo = 'foo' // uses [[Set]] semantics
  define bar = 'bar' // uses [[Define]] semantics
}

Honestly, it would make most people (talking about end users who develop products, not language designers or implementors) happy.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

I was very pleasantly surprised by TypeScript when code didn't break after extending the types and default values of certain props in my subclass (which are getters/setters in the super class), using class fields.

I love that TypeScript chose [[Set]] so that I was able to take advantage of super class getters/setters while still allowing my subclass definition to be clean and simple.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

JavaScript code will not break; typescript code has always risked breakage by adopting features prior to stage 3.

Define semantics are shipping, and the committee (including typescript team members) settled this question in 2017 - I’d rather we had decided on Set, but this is the consensus we arrived at.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

This is how the TS team will address this change: microsoft/TypeScript#27644 (comment)

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

JavaScript code will not break

Not sure why you'd say that. Changing properties from [[Define]] to [[Set]] semantics (f.e. converting JS to TS), or vice versa, can clearly break things.

For example, this is JS code:

class Child extends Parent {
  x = 0
}

The following TS code,

class Child extends Parent {
  x: number = 0
}

will be converted to use [[Set]], even if target is es2019. This is the output:

class Child extends Parent {
    constructor() {
        super(...arguments);
        this.x = 0;
    }
}

This is how the TS team will address this change: microsoft/TypeScript#27644 (comment)

Breaking people's code when they update to TypeScript 3.6 doesn't sound like a nice idea. I hope they leave [[Set]] as the default behavior, and only allow the new behavior as a compiler option.

[[Set]] encapsulates what JavaScript has already been for many years before class fields, and it is what people know. Why make such breaking changes to existing paradigms and mental models, replacing them with less convenient mechanisms?

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

typescript code has always risked breakage by adopting features prior

And TypeScript also proved people love and widely enjoy the feature ([[Set]] semantics), yet TC39 didn't mind sacrificing that.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Despite advocating for Set for years, I would be very surprised if even a percent of users ever cared about the difference.

@rdking

This comment has been minimized.

Copy link

commented Jun 17, 2019

How will you ever know? As is always the case, when bad decisions are made that don't completely break the language, they just end up in "the bad parts" lists, and people passively work around them. If the people who vocally frequent these discussion boards were to be considered a representative sample of the ES community (it's not, but for the sake of argument), then it would already be clear that more than 1% of us think that it makes little sense to use [[Define]] semantics on instance objects. It's not consistent with anything else in ES.

The fact that it's a new feature doesn't excuse or justify unnecessarily breaking known paradigms. No matter how much you try to brush away complaints by saying its already shipped, that's not going to improve anyone's opinion of the decisions or fix the common use cases that have been broken. Most of the disappointment comes from the apparent contradiction between TC39's desire to not break existing code while showing little to no caution toward breaking existing paradigms that are not directly related to the new feature.

All we want is to be able to declaratively define properties that will appear on each instance of the class. If Initialization is part of that definition then shouldn't it be just as immutable as the field being defined? By immutable, I mean that no one can dynamically alter the class definition to change the name of, remove, or otherwise alter the field declaration in the class definition. Even decorators don't do this. If a dynamic initializer is what is wanted, shouldn't that be done in the constructor? Even if avoiding the constructor is desired, the dynamic initialization should still proceed as if a human had written constructor code. That means [[Set]] semantics. The point is that instance properties shouldn't interfere with prototypal inheritance unless the developer wants it to on a case-by-case basis.


The rant above is a justification for [[Set]] semantics. If a similar justification for [[Define]] semantics exists, can someone please post it? NOTE: the FAQ contains no such justification, merely an explanation of the decision that was made. What I'm looking for is "WHY!?"

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

One justification, as I understand it, was that many committee members felt it important that all the code in the class body be declarative - in other words, if they see foo = 1, and nothing in the constructor deleting or changing it, that should be sufficient for them to know with certainty that an instance of that class has foo initially set to 1. Set semantics means they'd also have to traverse the entire class hierarchy - which could be runtime values in extends, so not statically knowable.

@rdking

This comment has been minimized.

Copy link

commented Jun 17, 2019

Thanks for the quick response. Hasn't that been faulty reasoning from the beginning? Every language supporting calculated members violates that notion unless the member is either private or overridden.

@rdking

This comment has been minimized.

Copy link

commented Jun 17, 2019

It just hit me. This is a problem of conflicting goals. Correct me if I'm wrong, but both of these appear to be goals for this proposal:

  • Any public, lexically scoped field declaration must override any existing inherited field of the same name from any superclass. (This is the only way to gain the certainty you spoke of.)
  • Field initializations must be dynamic to allow for constructor-less classes with initialized fields.

The problem?
Almost no one would use defineProperty to create a field unless they were either modifying the attributes of the field, creating an accessor, or intentionally trying to hide an inherited field. In all other cases, simple assignment is the defacto standard.
So on the one hand, you've got people trying to avoid constructor(){} but wanting to initialize fields.
On the other hand, every element in a class statement is a "define" and automatic override.
Therefore, you end up with people wanting to initialize fields outside the constructor and ending up defining new ones instead.

That means the goal of avoiding the constructor is completely lost to those simply trying to initialize base-class fields. The advertised ability to omit constructors for doing simple field initializations is broken.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Notably, tho, the conflict you describe only applies for those that care about Set semantics - which would only be those participating in an inheritance hierarchy that also uses setters.

@rdking

This comment has been minimized.

Copy link

commented Jun 17, 2019

You do realized you just described many framework, library and tool authors, right? Even though it's only going to be a small percentage of ES developers, what that small percentage does affects a large percentage of ES developers. You shouldn't be so quick to diminish secondary effects.

@hax

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I’d rather we had decided on Set, but this is the consensus we arrived at.

@ljharb There is no real consensus in community, several authors of big frameworks/libraries never accept [[Define]]. And there is also no real consensus in TC39, if read the meeting notes, you can find some guys in one meeting said he don't accept [[Define]] (or said if using [[Define]], it must use keyword), but the objectors just disappear for unknown reason in next meeting which "arrive the consensus". Is it a real consensus? I can never agree.

And assume if there is new member and delegates come into TC39 who never agree current design and refuse to allow it go to stage 4, what will happen?

@hax

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I would be very surprised if even a percent of users ever cared about the difference.

@ljharb This is the point. Most users don't want to care about it, but unfortunately it will affect their codes and life.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@hax I’m one of the ones that spent over a year fighting for Set, and ended up agreeing to consensus on Define, because the feature is important enough that letting it never happen wasn’t acceptable.

Once it’s stage 3 and two browsers are shipping it, it’s web reality - so it would be pretty strange for someone to object to stage 4 at that point. It could certainly happen - it just wouldn’t change anything.

@hax

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

One justification, as I understand it, was that many committee members felt it important that all the code in the class body be declarative

@ljharb I don't want to say their concern is not important. The real issue here is why other concerns from the professional programmers outside of TC39 can not be treat as same importance.

@hax

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@ljharb

because the feature is important enough that letting it never happen wasn’t acceptable.

We have some disagreement here.

  1. My opinion is such footgun is bad enough to veto the whole proposal
  2. Actually there are some ways to solve or mitigate the footgun:
    • Add keyword
    • Use [[Define]] on prototype then initialization on instance

Especially the second point, it's impossible to explain to the community why such fix is refused.

@hax

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

it just wouldn’t change anything.

At least it wouldn't allow such broken feature go to spec and may give a signal that other vendors might choose not land it.

@DanielRosenwasser

This comment has been minimized.

Copy link

commented Jun 18, 2019

I'd be more than happy to revisit [[Set]] semantics again. My feeling from speaking with a decent number of members of the committee is that many people are saying "Set was actually the right one, but I think it's probably too late."

I hear this over and over again, but it sounds so strange that we've concluded that. Maybe I'm wrong about this, but maybe we have more interest in [[Set]] than we've realized?

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@DanielRosenwasser even if the majority has concluded that (2 years later), theres still a few people i doubt have changed their position. Happy to discuss it next meeting.

@kaizhu256

This comment has been minimized.

Copy link

commented Jun 18, 2019

off-topic rant-warning - against excessive tooling

You do realized you just described many framework, library and tool authors, right?

i feel there's an industry glut of tooling (as opposed to product-development) folks and if they feel like their livelihoods are threatened ... they should be -- alot of it is redundant (plus no employer is happy hiring a js-developer for non-product-development reasons).

what if i told you most javascript web-projects could be done just as easily w/ minimal tooling (aside from a linter)? and that most people interested in javascript are non-technical, small-business-owner/design aspirants who see javascript as a "tool" (in the uncomplicated sense) for their startup ideas? is any of the tooling since es6 relevant to them? probably not ... its likely more cost-effective for them to stay with self-contained tools liked jquery/bootstrap/datatables/highcharts/etc that doesn't bitrot every 6 months.

my sentiment is many of you tooling folks are ultimately going to lose your jobs because:

  1. your c#/java/c++ derived tools and frameworks suck (for the non-technical folks who need it most)
  2. is mismatched/over-engineered for javascript's problem-space - baton-passing dynamic JSON-data between nodes
  3. the historical reasons for them (browser/engine incompatibilites) no longer apply.

javascript product-developers can get along fine, with-or-without most your toolings.

@rdking

This comment has been minimized.

Copy link

commented Jun 20, 2019

@kaizhu256 You weren't trying to bit rot the meaning of that quote, right? 😝

I'm an old school developer that prefers to let my only tool be the IDE I'm using and interpreter/compiler/linker for the language. However, most ES developers won't share my enthusiasm for knowing what every stitch of code in a project is doing. It's because most ES developers prefer to benefit from the work of others that the relatively small group of framework, library, and tool authors have such a big effect on the ES community.

Sure there are a lot of redundant tools out there, but even if there weren't, the fact that many developers depend on the works of others to complete their own projects means that something as seemingly trivial as this debate over [[Set]] vs [[Define]] will inevitably have a huge impact on the ES community, even for those who would prefer to ignore such minutia.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

@ljharb

One justification, as I understand it, was that many committee members felt it important that all the code in the class body be declarative - in other words, if they see foo = 1, and nothing in the constructor deleting or changing it, that should be sufficient for them to know with certainty that an instance of that class has foo initially set to 1. Set semantics means they'd also have to traverse the entire class hierarchy - which could be runtime values in extends, so not statically knowable.

@ljharb

Many things in JavaScript aren't designed to be statically analyzable, including accessors. If I run this.foo in my sub class, I had better take the time to know what it is doing. This is and always has been the nature of JavaScript, there's no need to change it.

However, TypeScript already helps us here: if this.foo is a setter/getter in a base class, TypeScript already knows about this (they did that work for us, the work you mentioned in the quote), and it will expect the subclass type to match the superclass type for the same property, regardless if it is a getter/setter.

IMO, your quote isn't a good reason to choose Define over Set. If I want static analysis of my classes, I can use TypeScript for that with plain JavaScript. It works great.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

Notably, tho, the conflict you (@rdking) describe (in #248 (comment)) only applies for those that care about Set semantics - which would only be those participating in an inheritance hierarchy that also uses setters.

@ljharb Correct. And it isn't that uncommon. Why do you think it is fine to break semantics for those people?

Think about it: "Cool, I can move these out of the constructor and remove the constructor.... What the hell? Why doesn't it work now?".

You agree with creating problems?

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

I’m one of the ones that spent over a year fighting for Set,

Sorry, missed that part. You shouldn't have stopped fighting for it.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

The existence of this feature is way more important than Define vs Set.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

I don't think so, because I can easily run this.foo = whatever inside a constructor. It's not super critical to rush it just because we can save keystrokes from deleting a constructor definition.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

That’s just public instance fields. Public static fields, and private fields, are not so trivially desugared.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

For private fields, it doesn't matter, there's no inheritance. So, we don't care about those in this case.

For static fields, I'd say it should respect inheritance too (use [[Set]]). I really don't see why not.

@trusktr trusktr changed the title TypeScript class fields have [[Set]] semantics. Use [[Set]] by default, add `define` keyword. (was: TypeScript class fields have [[Set]] semantics.) Jun 26, 2019

@rdking

This comment has been minimized.

Copy link

commented Jun 26, 2019

@ljharb

That’s just public instance fields. Public static fields, and private fields, are not so trivially desugared.

Really? In the constructor...

desugared public static field
this.constructor.field = value

desugared private field

//Assuming pvt is a WeakMap in a closure wrapping the target class
if (!pvt.has(this)) pvt.set(this, {});
let p = pvt.get(this);
p.field = value;
...
Object.seal(p);

Sure. Not a 1-liner for private, but still trivial. It's perfectly fine to use [[Define]] on private fields as this is fairly close to what is semantically happening internally: new internal slots being defined for the newly created object. However, for public fields, with it's long history of prototypal inheritance and the implications that come with this implementation, [[Define]] just doesn't fit how the language has historically been used. ...But I know I'm just preaching to the choir.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@rdking no, a static field is defined without the constructor ever having run. The desugaring requires you mutate the class outside the class body, which is not an ergonomic option (especially for class expressions).

Additionally, the .constructor property is configurable, so it's never reliable - thus any reliable desugaring must not rely on .constructor.

@rdking

This comment has been minimized.

Copy link

commented Jun 28, 2019

@ljharb I thought your statement was about how "trivial" a desugaring is for the various declaration types. No one said anything about ergonomics. The whole point of defining new syntax is to increase the ergonomics and simplify use of the common patterns, right? That still doesn't mean the desugaring is non-trivial.

As for .constructor, the related problems for static fields are only an issue for anonymous classes. However, in that scenario, static fields are of limited use due to the difficulty of accessing them. For all other cases, there's access to the variable holding the class constructor. In the end, the trouble you speak of, while it does indeed exist, is only an edge case.

@hax

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

That’s just public instance fields. Public static fields, and private fields, are not so trivially desugared.

@ljharb You (and some in TC39) just mix too many things into one, which make the proposal fragile and full of holes. For example private static fields also has footgun in some cases. Actually, as I said before, nearly every part of this proposal, syntax/semantic of public/private field of static/instance have at least one hole. Even every hole is small, the ship will sunk down finally.

@trusktr

This comment has been minimized.

Copy link
Author

commented Jul 8, 2019

a static field is defined without the constructor ever having run. The desugaring requires you mutate the class outside the class body, which is not an ergonomic option (especially for class expressions).

That's what we'll be doing to avoid [[Define]], so that doesn't help. I'd rather not have a feature that's going to cause problems.

As a professional engineer, writing SomeClass.foo = 'bar' outside of the definition is perfectly fine and not much slower at all, it doesn't waste much of my time. As a professional engineer, I can also not use the new features, and get the job done without complications.

The new syntax would be nice sugar, but in this case it is sour. As someone who will need to get things done at work, I'll gladly choose the out-of-class syntax which works perfectly well.

This is my weekend time, and I'm using it to share my desire for the correct feature, which is [[Set]] by default, with an optional syntax for [[Define]] (f.e. define foo = 123).

@glen-84

This comment has been minimized.

Copy link

commented Aug 11, 2019

@DanielRosenwasser even if the majority has concluded that (2 years later), theres still a few people i doubt have changed their position. Happy to discuss it next meeting.

Did this happen?

@hax

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@glen-84 There is no topics related to any part of class fields in July's meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.