Ball.js position/velocity Properties with getters/setters

[jonathanolson]

In https://github.com/phetsims/collision-lab/blob/master/js/common/model/Ball.js I see these are backed by independent writable x/y Properties, and a combined readable Vector2 Property. These are backed by ES5 getters/setters for both the independent versions AND the combined Vector2 versions.

I am fine with the ES5 getters/setters personally (opinion from years ago at https://github.com/phetsims/axon/issues/71#issuecomment-250332558), but I believe we reached a consensus that ES5 getters/setters should not be used that way (at https://github.com/phetsims/axon/issues/71#issuecomment-252047466). I'd like to discuss this again at developer meeting briefly before recommending something that I'm personally hesitant to recommend.

Additionally, for this case in particular, I'd recommend having the Vector2 Properties be the primary versions, with x/y DynamicProperties that are bidirectional for each component. This will allow setting either the full state OR the component state in ONE Property modification, as all of the Properties would be writable.

The code is commented as:

Separated into components to individually display each component and to allow the user to manipulate separately.

I'd also like a response on whether this is necessary to split into x/y Properties at this level. Can't this be on a view component so that it only modifies a specific component? Is there something from phet-io where we gain from having separate x/y? Having a single positionProperty and velocityProperty seems cleaner in general.

I'll be available to discuss all of this whenever (and the ES5 getter/setter part specifically at dev meeting).

https://github.com/phetsims/collision-lab/issues/153

[brandonLi8]

I'd also like a response on whether this is necessary to split into x/y Properties at this level. Can't this be on a view component so that it only modifies a specific component? Is there something from phet-io where we gain from having separate x/y? Having a single positionProperty and velocityProperty seems cleaner in general.

I looked over this again and it seems like we can remove having separate x/y properties. The only time these Properties are referenced in other classes is in BallValuesPanelNumberDisplay, which I suppose should display a DerivedProperty of the component of the position/velocity vector2Properties. When the user goes to edit the value via KeypadDialog, it seems like instead of passing in a Property to modify we could directly set one of the components, as you mentioned.

I'll investigate this locally.

[brandonLi8]

I investigated in the ball-vector2-properties branch. Un-assigning.

[pixelzoom]

This is probably related to @jonathanolson's question in Slack developer channel:

What's our current stance on PropertySet-like code setups? (e.g. if you have a Property as a field on an object, creating ES5 getters/setters without the "Property" suffix that forward to the Property). I recall being fine with that setup, but that others strongly preferred NOT having that due to clarity and readability

I replied:

Seems OK to me. But I prefer call sites that don't hide the fact that I'm setting a Property, so I generally don't add such ES5 get/set. And I don't like APIs that allows both foo.widthProperty.value = and foo.width =, especially when they're used interchangeably in a sim.

And @jbphet gave my reply a 👍🏻 .

[Denz1994]

From Dev Meeting on 09/24/20:

Are we okay to have getters/setters for Property? Devs agree that we should be consistent within their sim code. The use of Property getters/setters in this sim was based on dev preference. Using getters/setters with side effects should be up to developer preference going forward.

We will revisit at the next dev meeting for further input to refine exceptions or rules for the above thoughts.

[samreid]

I asked if it this wouldn't be as problematic if the IDE depicted object property setting differently than calling a setter, and @pixelzoom replied that would be a significant help. I tested in WebStorm and found it already by default depicts them differently:

Here's that code if anyone wants it for testing purposes:

class A {
  constructor() {
    this.x = 'hello';
  }
}

class B {
  constructor() {
    this._y = 'test';
  }

  set y( t ) {
    this._y = t;
  }
}

const a = new A();
a.x = 'bye'; // x is purple

const b = new B();
b.y = 'test'; // y is tan

[zepumph]

We decided that in general this pattern is convenience that isn't worth the cost of writing getters/setters (like in sim code). There are places where it is worth it, like in scenery like Node.visible. We are good with these cases. @jonathanolson will add this heuristic to the "coding conventions" part of the review checklist.

https://github.com/phetsims/phet-info/blob/master/checklists/code_review_checklist.md#coding-conventions

We agreed for this issue that this pattern wasn't necessary, and accessing the Properties directly made the most sense.

[jonathanolson]

Documented, and removed the ES5 getters/setters that were extraneous in the above commits.