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

Actively harms Class declaration maintainability #112

Closed skatcat31 closed 5 years ago

skatcat31 commented 6 years ago

This proposal has a problem of inconstant design paradigms for the same purpose that leads to a confusing footprint:

class Person{
  numberOfFeet = 2

  constructor(name){
    this.name = name
  }
}

const fredSmith = new Person('Fred Smith')

fredSmith.name == 'Fred Smith'
fredSmith.numberOfFeet == 2

[yesterday, tomorrow, today].map(date => {
  // there are some days fredSmith is using a vacuum cleaner
  // he will have an accident today
  if (today = new Date().getDate()) {
    this.numberOfFeet = 1 // This is not declared in the constructor! Oh woe is me trying to maintain this code and having to remember this edge case of another way to declare an instance variable
  }
}, fredSmith)

It is actively harming adaptability and cross team maintainability all in one. There is no typing so I know it's not the Java/C++ like version. Since the class declaration is so similar to Python/R, does that mean it's a static class variable? Which is it? Here the only way to understand that the numberOfFeet is an instance variable is to read the documentation for ES Classes that defines this behavior.

How would this be a boon to maintainability when if there is an argument to a class I may as well put it inside the constructor so that it's obvious in a single area what is happening?

class Person{
  constructor(name){
    this.name = name
  }

  nameChange(newName){
    this.name = newName // and a trip to the DMV
  }

  // possibly hundred of lines of code later
  numberOfFeet = 2
}

With this I now have to read the entire class to learn that a Person will default with numberOfFeet == 2 which makes maintenance horrible if we evolve and now we have 3 feet due to WW3. It's not in the key-worded area, so I have to do a find. If we stay with constructors then I merely have to read a constructor function to learn what modifications happen to this in a single place.

At that point for maintenance reasons we may as well refactor to just allow variable numbers of feet and enforce a linter in our project.

class Person{
  constructor(name, numberOfFeet = 2){
    this.name = name
    this.numberOfFeet = numberOfFeet
  }

  // the other possibly hundreds of lines of code
}

To find out what instance variables are available I should only have to read one key-worded method, not a whole class if the rest of the class does not apply. It'd only really help in that class that DON'T need a constructor because they accept no arguments and every instance will only ever be created with defaults:

class FordModelT{
  numnberOfWheels = 4
  numberOfDoors = 2
  windShield = true

  // several dozen functions later
  gasType = 'diesel'
}

Now again I might have to read the entire class because the keyword function can't just be CTRL+F and read from kerning to kerning. That may lead me to never knowing that FordModelTs take diesel and instead I crash because I send it to get a fill up with standard grade petrol. Remember good programs should be short, to the point, and built with some sort of easy to understand structure. This actively harms that because my instance variable aren't grouped nicely in a kerning level. They're grouped in multiple kerning levels. What a nightmare. Now I HAVE to use a linter if I want maintainable code, a step back from it's current maintainability in knowing that instance variables are in a specific area.

Is this really worth harming maintainability? I would never want to work on something if I knew it meant I would have to read EVERY class in their entirety just to see what member variables are present when I just call a constructor. Developers are lazy. This means someone will inevitably put an instance variable at some arbitrary point in the class just because that's where they were.

skatcat31 commented 6 years ago

For those that want to say 'These can only b at the top of a class', I did a find on 'top' and found no usage of the word there, meaning they may be defined ANYWHERE in the class

skatcat31 commented 6 years ago

If we limit it to the top of a class file before a method decleration(meaning bound arrow functions in the class would be fine before prototype methods) this issue becomes moot.

edit: No that's kind of dumb if we limit it to top of class. Linting tools could enforce them where you'd want them, but still limiting where you can declare as a programming language is kind of idiotic unless it's a keyword

jhpratt commented 6 years ago

This has been around in TypeScript for quite a while. Has anyone had an issue with it there?

skatcat31 commented 6 years ago

Which has? The class variables? Or limiting them to the top of the class?

jhpratt commented 6 years ago

The class variables. I'd argue limiting it to the top should be the job of a linter and/or style guide, not the language itself, barring technical reasons.

borela commented 6 years ago

Anyone with a Computer Science degree or training in another language would never understand that it is an instanced variable without knowing this prior.

Why not? This is similar to structs in C++, the only difference being that we have no concept of private(yet) or protected. You might have a different taste of how this should be declared(I do have many dislikes with js too) but saying people would not understand is a lie.

With this I now have to read the entire class to learn that a Person will default with numberOfFeet == 2 which makes maintenance horrible if we evolve and now we have 3 feet due to WW3.

Again, your example shows a stylistic preference, that is solved with a linter; In C++ some people prefer to declare public variables first, some prefer all variables last, use a linter to make the code consistent.

To find out what instance variables are available I should only have to read one key word method

Again, stylistic preference, use a linter and enforce your style through it.

skatcat31 commented 6 years ago

Why not? This is similar to structs in C++, the only difference being that we have no concept of private(yet) or protected. You might have a different taste of how this should be declared(I do have many dislikes with js too) but saying people would not understand is a lie.

Fair enough. Removed sentence.

Again, your example shows a stylistic preference, that is solved with a linter; In C++ some people prefer to declare public variables first, some prefer all variables last, use a linter to make the code consistent.

Enforcing stylistic preferences shows that it's confusing and harms the languages maintainability. If you have to use a tool to keep it maintainable and that everyone needs to read your rules to learn how your code is written it is not language level maintainability and is a false equivalence. It actually shows bad design decisions of adopters that don't do that. It is also something we currently avoid with the model of constructor being a single place for instance variable assignment. All ES developers NEVER have to look outside the constructor to see what modifications are being done to this and eventually what the initial starting point of an instance is. If it's not in a constructor, it's from some other dev and can't be relied upon to be in all instances. Simple as that. Linters don't matter for this. If I import a class I know every instance variable is able to be found in a single kerning level. That's vanilla maintainability. There is no way for it to be confused. I don't need a linter for a class to find the member variables. I just need it for the bad design decision JS has already made.

Examples of currently horrible maintainable things in JavaScript:

These problems are the biggest example of wanting a linter. It's because a bad design decision conflicts with a feature. That's just... ugh.

If we introduce a new feature that allows for bad design when we already had a feature that enforced good design, haven't we taken a step backwards?

skatcat31 commented 6 years ago

The class variables. I'd argue limiting it to the top should be the job of a linter and/or style guide, not the language itself, barring technical reasons.

While it helps avoid the problem, isn't this a step back from the current model of all instance variables are in a single kerning space?

bakkot commented 6 years ago

All ES developers NEVER have to look outside the constructor to see what member variables exists. If it's not in a constructor, it's from some other dev and can't be relied upon to be in all instances. Simple as that. Linters don't matter for this. If I import a class I know every instance variable is able to be found in a single kerning level.

let decorate = obj => { obj.field = 0; };

class Example {
  constructor() {
    decorate(this);
  }
}

...?

skatcat31 commented 6 years ago

@bakkot possibly proves my point more: They know it was passed to the decorate function explicitly. They only had to look in the constructor to find that all instances will be passed.

skatcat31 commented 6 years ago

@bakkot I have updated that paragraph to instead read the following

All ES developers NEVER have to look outside the constructor to see what modifications are being done to this

ljharb commented 6 years ago

@skatcat31 that's not entirely true; modifications could be made in any instance method or static method.

Public fields have been used in babel for years. This feature has more actual usage behind it than anything in the language previously, and in practice it helps maintainability, greatly.

If you want to know how a class works, you have to read its entirety - same with a function. If you find your classes are too large for this to be easy, I'd suggest writing shorter classes, and extracting functionality to other functions/classes. Forcing users to add a constructor only to create instance fields is boilerplate, and introduces a footgun around properly calling super with the right arguments.

skatcat31 commented 6 years ago

@ljharb Can't argue with that at all. The constructor does however enforce a static starting point for all instances regardless. The footgun is a problem that can't be addressed but that's also a problem with functions as a concept. I'll update with the fact that it's an initial starting point in a single place.

borela commented 6 years ago

Enforcing stylistic preferences shows that it's confusing and harms the languages maintainability.

Let me be clear, I too like to group variables in a single place and find having scattered all over the place bad BUT, I like to have the option to break the rules if I need to.

A common use case from other languages would be having private variables at the bottom and public ones at the top because most of the time, people are only interested in the public API.

To say that code style X is a mess is too personal, which is why I even picked my own eslint rules for my own projects because I could not stand airbnb's or prettier's styles.

skatcat31 commented 6 years ago

@borela I'm not certain I called them a mess. I'm saying introducing language features that actively obfuscate where modifications are being done to initial state because you're mixing it with an enforced language level construct is bad design.

We currently have a constructor paradigm in place that does not allow for overloading, and enforces calls to super before modifying this. It is also the only place a class instance receives and initial state. One stop shop for maintaining the initial state.

I'm guessing I'm wording this to aggressively towards linters directly, which couldn't be farther from the truth. I have no problem with linters(I actually like them). I have problems mixing paradigms. If these paradigms are mixed we've harmed the current footprint of a class and thus it's maintainability. We go from one footprint type(constructor and functions at the class declaration) to 3 footprints(constructor only, constructor & instance, instance only).

To be clear I'm as against this being introduced as I am allowing for overloaded constructors being introduced

class Example{
  constructor () {...}
  constructor (a) {...}
  constructor (a,b) {...}
}

I wouldn't want that either. If I wanted an extendable constructor I'd just use a dictionary initialization or conditional constructor arguments and type checking.

class Example{
  constructor ({...}) {...} // check conditionals
}

class Example2{
  constructor (a,b,c) {
    if (b) {...} // other cases, etc.
  }
}

This way I can always find a classes initial state, and I don't have to go poking around to find what constructor I failed in.

If overloaded constructors were allowed I'd be all for this proposal probably because then I could have a singular place to declare variables that then would be used regardless of what constructor you hit. The problem is ES as a language decided to disallow that, making this no longer a clarity helper, but a clarity harmer. Had the overloading been a feature than this would have been a great feature, but that's because in that case you have infinite footprints(1 constructor to X constructors). Infinite footprints makes default member variables a pain. This would be almost required then to just save the copy pasta that would come.

borela commented 6 years ago

@skatcat31 I understand what you are saying and I too like to have a pattern and detecting the initialization flow as fast as possible but limiting the declaration of properties to only the top of the class would be strange for anyone coming from other programming languages and it would prevent a common use case I often see where people put public variables at the top and private ones at the bottom.

From some of the sources I saw using this feature, the only time I had the issue you describe of being lost trying to find the where things begin and where they end was because the class itself was too big, it was doing too much, but that's solved with design patterns and breaking the code into multiple files.

skatcat31 commented 6 years ago

@borela you're right limiting them to the top is dumb. I have edited that comment to reflect which is something I should have done long ago, but we still can't walk away without addressing the fact you go lost in the first place on any class declaration regardless of size.

If I have the current model of only having a single constructor and I have an X line file with a Y line constructor and there can only be one constructor I know for a fact I need only read Y lines out of X lines to discover what an initial state of this class looks like. I can't get lost no matter how big the class is. Getting lost once is something that can't happen currently. It can only happen when you introduce one of the two patterns I spoke of previous so that modifications can take place in places other than the single constructor. It's always Y/X lines read right now.

While good developers and design patterns can be a thing you still got lost once showing not everyone will use them. This makes it again a harder to approach paradigm. You must now read X lines to know the initial state. Every. Time. While @ljharb brought up a good point that you need to read a class to understand the class, often times you merely care what properties exist on an instance because you are developing a function that accepts an instance for reference purposes, and does not modify it or need any functions from it. You only care about how you need to access the properties to make a control logic decision. Think of filtering an Array of People based on their hair color or an Array of Users based on what company they belong too. I don't need anything but the initial properties locations. Would you rather read X lines or Y/X lines to find that label?

hax commented 6 years ago

I found this issue is very enlightening.

I think the root cause of this issue is: there is a conflict between the declarativeness of field with the cohesiveness of the initialization. You simply can not have both. Most languages (like Java/C#) choose the former, JS (uptonow) choose the latter.

Is it the good idea to jump to the other side now? (Also note this is one-way, you can not go back.) I'm not sure. Sometimes we just too greedy for new thing, but forget the value of those we already have many years.

panlina commented 6 years ago

But don't you think it more maintainable if you don't even need to look into the constructor?

panlina commented 6 years ago

@hax Actually they don't conflict. Think about c++'s member initializer..

panlina commented 6 years ago

Why my comments lost once I commit, and appears once I commit another. :(

hax commented 6 years ago

@panlina

But don't you think it more maintainable if you don't even need to look into the constructor?

I think the discussion before already give the comparison.

Basically, you are just moving the initialization from the constructor to the class body (aka. just syntax sugar), no much benefit, but bring other footguns (like use [[Define]] semantic instead of [[Set]], see recent discussion in other issues to understand why it's a footgun) and make the initialization order much more complex to understand. (Do you know when the field initialization occurs? before super() or after? make sure every team members know it!)

Yes, in some simple cases (let's say 80% cases), you can omit the constructor, save a indent, and no super() call would bother you, a lot of benefits, huh? But in the other cases (20% cases), you now have to deal with both fields and constructor. Remember, if you have to debug with class hierarchy, there are now at least 2 times places you need to check, and again, do you know the execution order of them?

So, is that a good exchange at all? I'm not sure.

panlina commented 6 years ago

@hax

Why think it as syntax sugar, at the first place? It's limiting the scope if you think it as "just moving the initialization from the constructor to the class body", not to say that technically it is not, just as you pointed out about the different semantics.

It's neither appropriate or useful to think of field declaration as “another poor place to put initialization code, of which you lose the control of execution order”. When working with objects, there can be other things that is more important than execution order. On fulfilling certain requirements, it offers a way to define non-deterministic initialization of fields, which gives chances to other language features or optimizations to take place.

Field declaration can be a footgun, if designed badly, or has to be designed badly(due to existing language concepts or machanisms), but it's by all means another topic, not a reason to insist constructors as the only way to initialize fields.

skatcat31 commented 6 years ago

@panlina If you look through the thread I address that in the fact that we as a language only allow a single constructor, and that's due to previous language features like non typed and loose function footprint enforcement. It's why we have a single constructor. Our language just isn't built to support trying to figure out which version of a function to call at the moment, and as such we have a single constructor, just like we have a single function if we declare it in a function with prototype and use the new keyword(what classes are sugar over and add new features to with the keyword notation).

Because of this we have a single constructor and a defined order of operations within with regards to super should it exist. As such moving the deceleration outside of the constructor actively harms maintainability in the current JS ecosystem as a whole. Even another developer admits that with this proposal they can now get lost looking through classes to try and find "magic numbers", something our language has tried very hard to avoid. It will either be found on the prototype declaration or in the constructor is the current model for initialization. With this proposal we now move to finding instance members in the constructor, static members on the prototype, and instance members somewhere in the class chain. Now instead of having two very easy to find areas, the prototype modification and class constructor, both of which can be easily found(patterns exist for both cases that can find all instances across your code), we now have a non pattern matched place to attach instance variables that require a much more complex regular expression to find in the source code that requires look back logic to match kerning levels to make sure you're scoping correctly to check for any and all instance variables.

If nothing else our pattern matching goes from having to match constructor for a class file or extends Class then constructor and Class\.prototype\..*= to something much less manageable to find the assignment area of the value. Now instead of having to read a single area of a class declaration or having to grep over a project to find where the prototype is mutated, you also now have to read EVERY line of a class declaration.

skatcat31 commented 6 years ago

If we had multiple constructors this would be the former case that @hax mentioned, and at that point this becomes a helper. Sure it would still suck, but it's better than typing it 5 or 6 times, and worse yet it's consistent. However we as a language lack the tools to make that a reality since we don't have the compiler to enforce what version of the constructor is being called at all times.

hax commented 6 years ago

@panlina

just as you pointed out about the different semantics.

I already asked, how could you ensure all your team member know the difference and the consequence of it? Note, up to now, TS/Babel 6 always use [[Set]] semantic. And no mainstream introduction articles mention the difference and the traps because of it.

It's neither appropriate or useful to think of field declaration as “another poor place to put initialization code, of which you lose the control of execution order”. When working with objects, there can be other things that is more important than execution order.

Except applying decorator, could you tell me what's the "more important things" that we can't do now?

How everyone have to agree such "more important things" are really important than execution order?

Even decorator, we could apply decorators to methods/accessors, so why not only introduce a sugar for auto generated getter/setter which never introduce [[set]] vs [[define]] traps? Why insist on fields?

On fulfilling certain requirements, it offers a way to define non-deterministic initialization of fields, which gives chances to other language features or optimizations to take place.

What "important" language features or optimizations can not be achieved by getter/setter but can be achieved by fields without introducing traps?

Field declaration can be a footgun ... has to be designed badly(due to existing language concepts or machanisms),

You want a new feature, and others told you: no, such feature would introduce footgun because it conflict with existing language concepts or mechanisms. You answered: Yes, it has to introduce footgun due to existing language concepts or mechanisms. Others: ???

I just can't understand how this logic could lead us to any meaningful discussion.

but it's by all means another topic, not a reason to insist constructors as the only way to initialize fields.

What you are saying is just:

Assume introducing the feature could get 50 points for your "important" things. Assume introducing footgun could get -40 points. Assume losing constructors as the single place for initialization could get -20 points.

What you are trying to say is: because 50 + -40 > 0 && 50 + -20 > 0 , the feature is good enough to land.

PS. Many think such footgun is just -Infinity.

panlina commented 6 years ago

@hax

I'm afraid you are going off the topic. What I'm saying is that, introducing field declarations does not harm the maintainability by making it possible to have field initializations scattered at several places. I didn't say it does not harm the maintainability by other means. That's what this thread is mainly about.

You're assuming I insist on this feature. No, I didn't insist anything, if you read my reply in a fair way. I'm just disagreeing the saying that having field initializations out of constructors harms maintainability.

The math should be:

Assume introducing the feature could get 30 points for my "important" things Assume introducing footgun could get -n points. Assume losing constructors as the single place for initialization could get -0 points.

What I'm trying to say is: 30 + -0 > 0, but if 30 + -n < 0 , the feature is not good enough to land.

So, [[Set]] vs. [[Define]]? initialization order? Just speak out your disagreement and try to stop the thing.

But if you're saying that we should always keep constructors as the only place to initialize fields so field declarations should not be considered? No. This reason does not stand.

About 'the "important" language features or optimizations': That's quite obvious. I can list some.

nicolo-ribaudo commented 6 years ago

Isn't the initialization order of class fields well-defined?

skatcat31 commented 6 years ago

@panlina @hax both of you have strayed pretty far from what @hax originally wanted to point out into a conversation that no longer makes any sense. Let's get back on topic. This is dissent about active harm to maintainability of a single constructor language specifically in the form of moving declaration from definitive kerning level and single definitive method to multiple kerning levels and single definitive method. We don't have multiple constructors in JS, and so having this feature means more kerning searching by humans, the error prone portion of a program's source code.

In typed languages the type footprint plays a role in which function gets called as a constructor. It's something we don't have. JS has no way to determine which function you were trying to call based on the types passed and in what order. The best it can do is call the most recently defined pointer because it has no method of inference. This leads to the JS Class implementation having a single keyword constructor. Currently "class fields" and "initial states" are emulated using the prototype of the Class after creating the Class, but that's actually closer to a Static Class Variable(Static as in single instance to all children, not Static as in unchanging) and should be avoided.

let A = class {} // undefined, using a variable to hold a class is fine

A // class {}, empty prototype
A.prototype.test = 1 //1, set a class field
A.test //undefined, because it's not on A

let a = new A() // undefined, instancing of a class
a.test // 1, found on prototype of constructor, thus valid "class field"

a.test = 2 // 2, now an instance variable, confusing and often unexpected
a.constructor.prototype.test // 1, untouched as expected

delete a.test
a.test++
a.constructor.prototype.test // 2, static class field modified

However this is NOT the same as the proposal. The proposal is instead

Class A{
  a = 1;
}

which might be seen as a boon, but now carries the weight of which is it doing? Modifying a class level static(prototype mutation by designer of class), or a local instance variable? This would need to be well defined which it is. But now it will always carry that problem of "which was it that JS did?" which is ALREADY the most convoluted part of learning JS coming from any other language. Which of the following would you think the proposal was doing under the hood?

Class A{}
a.prototype.a = 1;
Class A{
  constructor(){
    this.a = 1;
  }
}

This is now something to be learned. Not a big problem, it could probably be addressed by a single sentence:

This creates an instance variable

Okay good now we know it's the first. Let's go write some code!

// Admittedly this is a bad example, but you should be able to tell why
Class Homework{
  a = 1;
  constructor(b){
    this.b = b;
    this.e = 'You can\'t set me';
  }
  d = 'test';
  add(){
    return this.a + this.b + this.c;
  }
  print(){
    console.log(this.d);
  }
  subtract(){
    return this.a - this.b;
  }
  c = 2;
  set e(){
    throw new Error('e is a private instance. You can\'t set it, you can only look at it.');
  }
  get e(){
    return 'You see me instead'
  }
}

Okay good. Wait. That looks awful. What kerning level is something set at? Guess we'll need to introduce behavioral driven development as part of the beginning lectures to avoid code obfuscation.

This is what I mean by it actively hurts maintainability. Without proper practices we now have multiple kerning levels to asses to find that 'magic number' which is something we usually try to avoid.

If we had typing and multiple constructors I would see NO problem with this proposal whatsoever because it cuts back on the developer error of copy pasta assignments in constructors

class A{
  constructor(String name){
    this.name = name;
    this.feet = 2;
    this.reasonForDifferentFeet = null;
  }

  constructor(String name, Number feet, String reason){
    this.name = name;
    this.feet = feet;
    this.reasonForDifferentFeet = reason;
  }
}

This code could easily be cleaned up by this proposal as follows:

class A{
  feet = 2;
  reasonForDifferentFeet = null;

  constructor(String name){
    this.name = name;
  }

  constructor(String name, Number feet, String reason){
    this.name = name;
    this.feet = feet;
    this.reasonForDifferentFeet = reason;
  }
}

However we don't get that typing, and as such we wouldn't benefit from 'less code repetition errors'. Instead we introduce 'more places for the same code' which leads to other problems already example because of the following:

The most common bug in programming is the programmer

hax commented 6 years ago

@nicolo-ribaudo

Isn't the initialization order of class fields well-defined?

Everything can be well-defined. But we should ask how average js programmers can handle them in daily developing.

littledan commented 5 years ago

There's a wide variety of JavaScript code patterns that people can write. Whenever we introduce new features, people can keep using the old ones, and they can mix things together. Given how popular public field declarations have been in transpilers, I think these are worth it for JS programmers, even if they introduce this sort of diversity.

TC39 decided that field declarations are worth this cost when we promoted the feature to Stage 2, and I'm under the impression most of the people commenting on this repository share that point of view as well, so I'm closing this issue.