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

No public instance property declarations #1

Open
ljharb opened this Issue Nov 2, 2018 · 27 comments

Comments

Projects
None yet
3 participants
@ljharb

ljharb commented Nov 2, 2018

It’s long since been determined that a declarative way to declare own per-instance public properties, outside the constructor, is a very critical need. One example is state in a react component.

How does this proposal meet this need (committed to by stage 2, wherein the committee agrees that certain problems/use cases will be solved by a proposal).

@mbrowne

This comment has been minimized.

mbrowne commented Nov 2, 2018

Related: #2

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

@ljharb You know I'm pre-disposed to logical thinking, right? Help me understand why the constructor is insufficient for this purpose.

The fact that react does things this way is only due to the fact that a proposal allowing this made it so far. If that proposal is replaced in favor of a proposal that doesn't yet allow for it, that doesn't prevent people from continuing to use the babel cross compiler to gain that feature. Neither does it prevent people from simply reverting to using the constructor for that purpose.

@ljharb

This comment has been minimized.

ljharb commented Nov 2, 2018

React did things that way long before any class fields proposal existed.

The constructor is insufficient because requiring it introduces the footgun - one which has caused actual bugs in production at airbnb and other places many times - of "forgetting to call super, or to call it with the right arguments". In other words, the powerful feature is allowing omission of the constructor for common use cases.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

@ljharb Well dang.... That's actually sounds like a good argument....

Thinking about it...

  • Forgetting to call super is kind of a non-issue since the engine will throw a ReferenceError as soon as someone tries to construct the subclass. So I'm not so bent about that one.
  • Calling super with the wrong arguments can indeed be a PITA, but if your super needs you to call it with arguments, you have no choice but to use a constructor. "Fields" can't save you from that.

What exactly was the nature of the foot-gun for airbnb?

@ljharb

This comment has been minimized.

ljharb commented Nov 2, 2018

Forgetting to call super is kind of a non-issue

This is only surfaced as a problem early if someone accesses this in the constructor - but to be fair, for this use case (this.x = y), that would be the case. So I agree this isn't really relevant to public fields.

Yes, I realize that if your superclass needs a different argument signature, you are forced to have a constructor - the goal is for that to be the only case where you need one.

The details aren't particularly relevant, beyond that devs often failed to make the constructor take the right (read: the same as the superclass) arguments, and also failed to pass them (read: the arguments the superclass was expecting) to super().

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

Let me see if I'm understanding you correctly. You're saying:

Problem:

  • when some developers write subclasses, they fail to take into account the needs of the superclass when coding the constructor function.

Solution:

  • elide the need to create the constructor function.

...and this should work due to the argument forwarding done by the default constructor.

Is this right?

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

BTW. Tried this in chrome...

>  class A {}
<- undefined
>  class B extends A {
       constructor() {}
    }
<- undefined
>  new B
x  >Uncaught ReferenceError: Must call super constructor in derived class before accessing `this` or returning from derived constructor
       at new B (<anonymous>:2:16)
       at <anonymous>:1:1

In other words, you don't even need to access this. Just returning without calling super is a foul.

Question: Can super be elided? What if the absence of super at the time the function is parsed causes the constructor to be generated with a super call injected as the first statement in the constructor with all the subclass's arguments passed?

@ljharb

This comment has been minimized.

ljharb commented Nov 2, 2018

Yes, this is right.

To your other point, if you fail to call super but return an object it works fine.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

Ok, then the problem really isn't with the constructor. You know, I wish gun manufacturers would put as much effort in keeping their guns from being misused as you're wanting to put into language features that make it ok developers to be sloppy. ;-)

I've got an alternate possible solution. Contrive a syntax that allows a super call to be made automatically passing all of the arguments from the constructor. That would conform much better to the way the language works.

@ljharb

This comment has been minimized.

ljharb commented Nov 2, 2018

The community has been using public fields in babel for 4 years, in multiple ecosystems - I think it's pretty clear that what is required is a non-constructor method of doing so.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 2, 2018

No. Sadly that's not clear at all. What's clear is that the non-constructor approach is viable. However, that approach is like using a sledge hammer to hit a shoe nail. I'm offering an approach significantly more surgical, and in line with the nature of ES.

I would be able to consider relenting only if my suggestion were first give a fair chance. It's not the constructor, but rather super that's stopping programmers from safely coding sloppily based on what you said. My best solutions are to either:

  1. auto-inject super into constructors of subclasses, passing along all constructor arguments, or
  2. contrive a syntax for calling super with all constructor arguments automatically passed

I can only consider adding "fields" after both of these approaches have been tried unsuccessfully. This proposal still has the "why not fields" document.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

This proposal is philosophically against fields, but it's not against properties. An instance property (i.e. a property that exists only on the instance, not the prototype) is still a property. And although I like the idea of being able to do dynamic initializations if feasible, this proposal would still be more palatable to me if it provided some syntax to declare instance properties even if only constant values (i.e. pre-defined or primitive values) could be used to set the property's initial value. (Given this approach, any fancier initialization, e.g. this.x = someFunc(), would have to be done in the constructor.) For example:

var x, y;  // instance variables
prop myPublicProp = 1  // public instance property

where the myPublicProp declaration would be 100% equivalent to doing this.myPublicProp = 1 in the constructor.

P.S. I'm far from sold on the syntax I'm proposing, and syntax issues like this are a major reason I still prefer the class fields proposal overall, but I still think some sort of syntax like this would improve this proposal.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 3, 2018

@mbrowne Nice try.
You might want to read the updated README.md. This proposal is also against the class definition modifying things that are not directly a product of the class keyword. The execution context of the constructor is not a product of the class keyword, so nothing in the class definition should be able to modify it.

That being said, it's not like I'm 100% against constructor-less instance properties. It's just that:

  • They introduce unnecessary timing complexities.
    • Fortunately those seem to have been resolved by working through class-fields
  • They introduce an immediate foot-gun if [[Define]] semantics are used
    • Solved by using [[Set]] semantics.
  • When using [[Set]] semantics, decorators may not be able to be applied
    • No solution exists for this one. It's just the nature of the design.

What I am saying is that @ljharb characterized the problem clearly enough for me to understand. Since the problem is with sloppy use of super, the most appropriate solution is to do something to clean up that slop! Trying to omit the constructor just because of that sloppy programming is like throwing out all the carpet in my house because I dropped cracker crumbs on the floor. I'm just saying why go through the complex expense of ripping out the carpet when I've got a working vacuum cleaner?

It's also not like I haven't considered what it would take to implement constructor-less instance properties. I just don't see any advantage at all in doing so. That adds complexity to the language for no gain. However, if there is no better limiter for the sloppy programming than that, then I would make the appropriate adjustments. I'm not unreasonable about this.


Here's what I'm suggesting:

class Base {
  constructor(arg1, arg2, arg3) {
    if (arg3 === undefined) throw new ReferenceError("All arguments are required");
    this.biv1 = "fu";
  }
}

class Sub extends Base {
  constructor(arg1, arg2, arg3) {
    //Don't bothering calling super. The engine will do it like this `super(...arguments);`
    this.siv1 = "sna";
    this.siv2 = "bar"
  }
  print() {
    console.log("I think non-constructor instance properties are ${this.biv1}${this.siv2} where ES is concerned");
    console.log("They would make class definitions be ${this.siv1}${this.biv1}.");
}

(new Sub).print(); //Look ma! No Errors!

I'm saying this should just work. There won't be any backwards compatibility issues with this, and code that's currently broken because of it would just start working properly. If there's something I missed that makes this non-viable, please tell me.

@ljharb

This comment has been minimized.

ljharb commented Nov 3, 2018

It’s not just about super. The point of class is to be more declarative about instances, prototypes, and the constructor (static things). Class fields are declarative; code in the constructor is imperative.

The solution is providing a declarative way to obviate the need for an explicit constructor, one use case at a time.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

The execution context of the constructor is not a product of the class keyword, so nothing in the class definition should be able to modify it.

Earlier you argued that the class members only belong on the prototype, but I assume you meant with the exception of static properties and methods. Stating the obvious here, but the product of the class keyword is a constructor function and its prototype. I do understand your reservations though...there are significant differences between declarations that affect the constructor function and prototype and introducing new ones that affect what happens when instances are created.

If there's something I missed that makes this non-viable, please tell me.

Well it's a significant break with current functionality for one thing. I feel like the right time to propose the ability to omit super() would have been when the ES6 classes proposal was being discussed. I agree that it would be good to be able to omit it and have it just work, as you described. (This goes back to the lack of planning I discussed in other threads...if the need for class property declarations had been considered more carefully back when ES6 classes were still a proposal, maybe they wouldn't have decided to require explicit super() calls.) I was not following the TC39 discussions about ES6 classes at the time, but I'm sure that the rule that you have to explicitly call super() was discussed at great length. I imagine the reasoning is that it's better to be explicit about it rather than implicit behavior happening behind the scenes. And of course we don't just have to speculate here; I'm sure the reasons for this decision are documented.

That said, perhaps it would technically still be possible to allow omitting super in a future JS version as you suggested. But it might not be easy and wouldn't necessarily be a good idea or even practically possible at this point...

And anyway, I would still agree with @ljharb that class declarations should include a declarative way of creating public instance properties. It's not accurate to claim that this proposal has feature parity with the class-fields proposal when it lacks public instance property declarations.

Although it's not my preferred option, here's a possible way around this: you could allow public property creation (using something like the prop keyword I suggested) but restrict initializers even further so they can only be primitives. That way prop declarations would create prototype properties, but as soon as you set them to a different value, they would be shadowed by instance properties. I believe you previously said you believe all properties should exist on the prototype, so perhaps you'd be more amenable to adding that feature.

@ljharb

This comment has been minimized.

ljharb commented Nov 3, 2018

@mbrowne that would not be sufficient - my primary use case is installing an object as an own “state” property in airbnb’s react components, declaratively, without the constructor.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

@ljharb Right, it wouldn't be ideal for React. But it would still be an improvement over the current version of this proposal, which lacks any kind of declaration syntax for public writable properties. (Not that the writable part is necessarily a requirement for React in theory, as long as it's still configurable, but it might need to be writable to work with React as currently implemented...I can double-check later.)

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

I realize the whole idea of instance property initializers goes against @rdking's goals for this proposal, but FWIW I think at least some of the issues with them could be avoided by a policy similar to what PHP has for their properties:

This declaration may include an initialization, but this initialization must be a constant value--that is, it must be able to be evaluated at compile time and must not depend on run-time information in order to be evaluated.

http://php.net/manual/en/language.oop5.properties.php

This would mean it would still be possible to do something like state = {...} as long as the ... part didn't include something dynamic like a function call.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

@rdking What will be the proposed rules for initial values for readonly properties? I searched the spec in this repo for readonly but it's not there.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 3, 2018

@ljharb

It’s not just about super. The point of class is to be more declarative about instances, prototypes, and the constructor (static things). Class fields are declarative; code in the constructor is imperative.

None of this quote rings true. Warning, the reasoning is long-winded.

First, nothing about the existing class keyword does anything to make instances more declarative. In fact, I'm not entirely certain what that means when the instance wasn't created from an object literal. Nothing about the class keyword does anything to instances at all. It only has 2 products currently: the constructor, which is either provided by the developer, or defaulted by the engine, and the prototype. The instance object isn't modified during construction unless a developer provides a constructor.

So, let's not go round in circles about this. Can you give me a link to meeting notes that occurred while discussing the creation of the class keyword that shows manipulation of instances was part of the original intent? I'll even accept anecdotal evidence. If that was really the case, then it becomes a lot easier for me to see doing such a thing. Right now, it just doesn't make any sense.

Second, there's no particular problem for which class-declared instance-properties is the best solution. Further, the implementation TC39 would use (the one from class-fields) is actually an imperative statement disguised as a declarative statement. This is evidenced by the fact that its effect is not guaranteed to produce the exact same value in every class instance created. So that's just moving an imperative statement somewhere where it doesn't belong. That's why there are so many edge cases that had to be considered just to make it work properly.

Again, I'm not giving a hard no to this. I'm just saying that there's not sufficient justification for the approach you're requesting. The foot-gun problem is solved by defaulting the super call when it is required but absent. The way to be more declarative about instances is to create them as object literals. Every other approach to creating an instance is imperative. That's just the nature of instances.

Got another argument? I'm still open. Try emotional or use case appeals. I really will consider anything if it's that important. But remember, the problem this proposal is intended to solve is language support for data and function encapsulation. I have a strong need for some equivalent for protected, but I'm not adding that to this proposal since it's a separate issue. I am, however, making sure the design includes room to implement such a thing. It'll be one of the follow-up proposals should this one be accepted.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 3, 2018

@mbrowne

Earlier you argued that the class members only belong on the prototype, but I assume you meant with the exception of static properties and methods.

Bad assumption. To me, a class definition looks identical to a prototype save for the lack of commas between declarations, and the presence of static elements. The static elements can be excused since there's no declarative approach to add properties to a function object before its declaration is complete. That's precisely what it seems like static elements do to the constructor. My statement includes the fact that the constructor is a property of the prototype. Modification of properties of a prototype object is modification of the prototype. Sorry if my wording made that confusing. This is why in later posts, I began using the expression "products of the class keyword".

Well it's a significant break with current functionality for one thing. I feel like the right time to propose the ability to omit super() would have been when the ES6 classes proposal was being discussed.

I agree.

I imagine the reasoning is that it's better to be explicit about it rather than implicit behavior happening behind the scenes. And of course we don't just have to speculate here; I'm sure the reasons for this decision are documented.

I would agree with that assessment as well, but that would require requiring programmers to stop being sloppy. I have no issue with requiring that (another reason why I see no merit in instance-properties), but for some reason TC9 doesn't seem to agree that developers should be aware of the gotcha's of a language and simply be careful and do a lot of testing. How is it the language's fault if a developer is sloppy?

I would still agree with @ljharb that class declarations should include a declarative way of creating public instance properties.

Like I said to @ljharb, nothing about an instance is declarative unless you're making it using an object literal. The very act of creation of an object literal is an imperative act.

It's not accurate to claim that this proposal has feature parity with the class-fields proposal when it lacks public instance property declarations.

Can you point out where this proposal makes that claim so I can fix it? To my knowledge, that claim isn't being made.

Although it's not my preferred option, here's a possible way around this: you could allow public property creation (using something like the prop keyword I suggested) but restrict initializers even further so they can only be primitives.

This proposal already has public properties. That was the first addition I made. I'll reserve the discussion about whether or not they need a keyword to #2. As for restricting the initializers, I've considered that, but I'd like to discuss the issue before deciding the best approach. I know what the foot-gun is there, and I have a few surgical solutions for it as well. If you want, open a new thread about that.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 3, 2018

@ljharb

my primary use case is installing an object as an own “state” property in airbnb’s react components, declaratively, without the constructor.

That's the kind of thing I'm looking for to help your argument. I've got a solution for doing that using public properties.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 3, 2018

@mbrowne
The readonly keyword is part of a different proposal I'm working on since it's useful regardless of what proposal gets accepted. When it's ready, I'll open it up for discussion as well.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 3, 2018

My statement includes the fact that the constructor is a property of the prototype.

Well that's kind of an afterthought as far as the JS engine is concerned; the constructor function is the primary artifact. The prototype is a property of the constructor. The constructor property on the prototype has its uses to be sure, but most things still work fine if it's absent. But anyway, I think you've made clear what you view as the purpose of class declarations and we simply don't agree.

@rdking

This comment has been minimized.

Owner

rdking commented Nov 4, 2018

I get that. Having a history with more than 15 OO languages, every time I've seen something like a class, it was basically a rubber stamp used to create objects, a static template. After seeing @ljharb's last comment about a need from airbnb, I get what you're really after. Instance property declarations can solve that, but still isn't what I think of as the best solution for the problem. I'm in the middle of writing up issue #3 to discuss possible alternate solutions. The good part is that for what I'm thinking, existing source code would change very little if at all.

@mbrowne

This comment has been minimized.

mbrowne commented Nov 5, 2018

I'm not sure if this is relevant for this proposal anymore, but I wanted to respond to this:

When using [[Set]] semantics, decorators may not be able to be applied

  • No solution exists for this one. It's just the nature of the design

There might be a solution. It's definitely possible to allow decorators to return a property that uses either [[Set]] or [[Define]], depending on what the developer wants. The part I haven't worked out yet is what should be passed via arguments to the decorator function. Let's suppose that the current version of this PR will be the real spec for decorators:
tc39/proposal-decorators#165

This would mean that you can return either kind: "field" for [[Define]] semantics or kind: "initializer" for [[Set]]. Where it gets tricky is if you have instance property declarations and you want them to use [[Set]] by default, as you and I and many others have advocated for. I think the return value part is straightforward, e.g. if you have this:

class Base {
  ...
  get x() {...}
  set x(v) {...}
}

class Sub {
  x = 2
}

And you want to use a decorator to affect only x = 2 declaration in the subclass and not the getter/setter from the base class (which would mean that you don't want the default [[Set]] semantics):

class Sub {
  @myDec
  x = 2
}

Then the decorator would need to return something like this:

function myDec(/* just an element descriptor here as per current spec? or multiple arguments? */) {
  // obtain `key` and `initializer` from arguments
  return {
    key,
    kind: 'field',
    placement: 'own',
    descriptor: {
      enumerable: true,
      writable: true,
      configurable: true
    },
    initializer
  }
}

As you can see, I'm still fuzzy on the full details of the API. It's a bit tricky because I'm in favor of instance properties that are created in the constructor (as in the current class fields proposal), and although by default the engine would create a new property descriptor for fields on base classes, it wouldn't (by default) create a property descriptor for overridden properties on subclasses. It's hard to explain but I think you know what I'm referring to, since I believe it's the same issue you're raising as a reason to avoid instance property declarations entirely.

It's entirely possible that I'm missing something important here, but my current assessment is that this is a problem to be solved by the decorators proposal, and class properties shouldn't be forced to use less-preferred semantics because of this issue. And I think it's solvable on the decorators end.


Just to finish my thought, this would be the default for Sub.x, without any decorator:

  {
    kind: 'initializer',
    placement: 'own',
    initializer() { this[key] = initializer.call(this) } 
  }

Note the absence of a descriptor here. I noticed that Daniel's example here still has a descriptor for an initializer element. (I'm not sure what the use case for that would be if [[Set]] were the default semantics, but given that [[Define]] is the default for class-fields I suppose that would be relevant in case you want a decorator that uses [[Set]] but falls back to [[Define]] if the property doesn't exist yet and uses your desired property descriptor in that case.)

@rdking

This comment has been minimized.

Owner

rdking commented Nov 5, 2018

I get what you're going after, but that's a contradiction. A "field" decorator is a "field" decorator because it does something to the class definition based on the field it decorates. If the decorator isn't passed a field definition, it's essentially a finalizer or initializer. That initializer can only return descriptor info. So it would basically be an initializer or finalizer that defines a new property. I think that's distinctly not the same as the original intent of a "field" decorator. That's why I said there's no solution. There's probably a work around or 2 like what you're suggesting, but that's not a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment