Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Default placement of instance properties (prototype vs. instance) #33

Closed
mbrowne opened this issue Jul 31, 2017 · 6 comments
Closed

Default placement of instance properties (prototype vs. instance) #33

mbrowne opened this issue Jul 31, 2017 · 6 comments

Comments

@mbrowne
Copy link

mbrowne commented Jul 31, 2017

It looks like in the current proposal, public properties declared without the static modifier are added to the instance by default, i.e. this:

class Demo {
    x = 0
}

Translates to this:

class Demo {
    this.x = 0
}

I'm a little confused when looking at the spec though, specifically this part of DefineField() (last step):

Perform ? DefinePropertyOrThrow (F, fieldName, desc).

I may be reading it incorrectly, but I thought that F was a reference to the constructor function / class - makes me wonder if it was a typo and it was supposed to beO there instead of F, but I'm not well-versed in reading these specs so maybe not.

In any case, assuming my understanding is correct that properties will go on the instance by default, I wanted to bring up two previous threads on this subject:
tc39/proposal-class-public-fields#38
tc39/proposal-class-public-fields#59

People have brought up a number of good reasons why it would be good for properties to go on the prototype by default. Here's an example use case where the difference matters:

class Entity {
  constructor(attributes) {
    this.assignAttributes(attributes)
  }
  
  assignAttributes(attributes) {
    for (let key in attributes) {
      if (key in this) {
        this[key] = attributes[key]
      }
    }
  }
}

class Cat extends Entity {
  name = null
}

const garfield = new Cat({name: 'Garfield'})

garfield.name  // null

If the fields are only defined on the instance, then the parent class has no way of knowing about them in the constructor.

I saw @littledan's comment in #3 that the current proposal leaves open the possibility of modifiers (e.g. own and shared) to explicitly specify where properties should be defined. I think that's a great idea, but I still think the prototype is a better default than the instance, at least for primitives. It could of course be problematic for object and array properties - those shouldn't be shared by default among instances, and if they were it would be surprising and a source of bugs. But it would be simple enough to set all non-primitives to null on the prototype and set their real initial values in the constructor (perhaps only if the parent constructor(s) didn't already initialize them - not sure about that part). Actually, even primitives could be set to null on the prototype if there is some reason that it would be better not to set their initial values there (although I can't think of one). The main concern I wanted to bring up is that the properties declared inside the class should somehow be visible on the prototype -- even if only the property names and not their real default values.

There may be other, better solutions to the use case above. I suppose what I'm really advocating is some kind of ability to do reflection on instance properties from parent classes or outside the class scope entirely (without needing to actually create an instance to do it). So I'd be interested to hear any alternative suggestions, or further explanations as to why the current proposal is as it is.

@bakkot
Copy link
Contributor

bakkot commented Jul 31, 2017

I agree there's a typo in the spec text; both O and F there are unbound and should instead be receiver. You're correct that the intent is for fields to be defined on instances.


If the fields are only defined on the instance, then the parent class has no way of knowing about them in the constructor.

Fields are installed before the body of the constructor executes (or at the start of the super call in the case of derived classes): in other words, they're already present on this by the time it's possible to refer to this. So your example would work fine, I think, and end up with garfield.name === 'Garfield'.

In any case, this has been discussed at length already in tc39/proposal-class-public-fields#59. Ultimately, putting fields on the prototype seems likely to do more harm than good, even if there are use cases for it.

@mbrowne
Copy link
Author

mbrowne commented Jul 31, 2017

Thanks for the explanation. Ah, so in this proposal, the properties are initialized before the call to super()? I thought the declaration of Cat above was equivalent to this:

class Cat extends Entity {
  constructor(attributes) {
    super(attributes)
    this.name = null
  }
}

This is also the only way to transpile it to ES6 with conventional class syntax, since you can't do this.name = null before calling super()...it's how Babel currently does it. And in this version garfield.name === null.

But if the real implementation will do this behind the scenes somehow:

class Cat extends Entity {
  constructor(attributes) {
    this.name = null
    super(attributes)
  }
}

Then yes, that would solve it.

Alternatively (or in addition), maybe there could be some methods on Reflect that would have access to instance property definitions prior to the call to super().

@bakkot
Copy link
Contributor

bakkot commented Jul 31, 2017

Sorry, I'm mistaken, instance fields on subclasses are initialized after the call to super exits, not when it enters. So you're right, that wouldn't work.

@ljharb
Copy link
Member

ljharb commented Jul 31, 2017

It kind of seems like your assignAttributes implementation is brittle; specifically for this reason. Typically I'd expect these attributes to all be stored in an object or Map that's under a single property on this.

@mbrowne
Copy link
Author

mbrowne commented Aug 1, 2017

I think it's reasonable to want to implement this idiom as written...basically the idea is to create a plain old JS object but with the ability to set all the parameters at once in the constructor - and ignoring any extraneous properties that might be passed in. I'm not sure how using an object or Map would help with this particular problem, unless you mean passing the attribute map as an argument to the constructor like this:

class Entity {
  constructor(attributes) {
    this.attributes = attributes;
  }
}

class Cat extends Entity {
  static initialState = {
    name: null
  }
  
  constructor(attributes) {
    super({...Cat.initialState, ...attributes})
  }
}

...but that defeats the purpose of using a base class to assign the attributes (which was intended to reduce boilerplate code). And it allows invalid attributes to be added to the object. So I guess I'm just not following you.

Anyway, this is just one use case for wanting to reflect on instance property names. I can imagine other potential scenarios...for example, if you had a constructor with required parameters, with the current proposal the only way you could find out the property names from outside the class would be to actually create an instance (requiring specific knowledge of how to call that particular constructor).

But there are many ways to accomplish reflection, and I take your point that defaulting to putting properties on the prototype could be problematic. So perhaps I should close this issue and file a new one specifically about reflection.

@mbrowne
Copy link
Author

mbrowne commented Aug 17, 2017

Closed in favor of #36.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants