Closed samreid closed 2 years ago
samreid
commented
2 years ago Eventually we may redo the type hierarchy in #343 but until then this is working great:
declare type ReadonlyProperty<T> = Omit<Property<T>, 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>;We can add it to axon once axon supports TypeScript. In fact, I think I could add it now since JS sims won't depend on it. Testing...
samreid
commented
2 years ago This patch adds ReadonlyProperty.ts to axon and demonstrates a use case:
For convenience, here is the type definition ReadonlyProperty.ts:
// Copyright 2021, University of Colorado Boulder
import Property from './Property.js';
type getValue<T> = {
get value(): T
};
type ReadonlyProperty<T> =
Omit<Property<T>, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
& getValue<T>;
export default ReadonlyProperty;And a screenshot showing how you can get values and link but not reset or change the values:
It appears that adding this TypeScript file to axon would not disrupt any pure JavaScript sims, so I think it is safe to add despite our "no TypeScript in common code repos yet" policy. This is because this new file would only be used by TypeScript simulations (since it is a TypeScript *.ts type declaration). For JavaScript sims, it would neither be picked up in the webpack step nor would it require a tsc step. However, I think it would be best to get feedback and approval before adding something like this to common code. @pixelzoom and/or @zepumph can you please review and advise?
pixelzoom
commented
2 years ago Re:
> type ReadonlyProperty<T> =
+ Omit<Property<T>, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
+ & getValue<T>;I guess I can see how this might be considered OK from a structural-typing perspective. But from an OOP perspective, it's backwards - ReadonlyProperty would be the base class, and a "writeable" Property would extend it to make it mutable.
Re use cases... I see how declaring a constructor/method parameter as ReadOnlyProperty can be used -- you can pass in a Property argument, and the client is limited to the subset of the API that deals with observing and reading the Property (the API is narrowed). I'm not clear on how ReadOnlyProperty would be used in the more common case, where a class has a Property field that is private/protected writable, but public read-only. Is something like this what you had in mind? :
class Thermometer {
// Writeable by this class and its subclasses.
protected readonly temperatureProperty: NumberProperty;
// Requires an additional public reference to someProperty that is read-only ??
public readonly temperatureReadOnlyProperty: ReadOnlyProperty<number>;
constructor(...) {
this.temperatureProperty = new NumberProperty(..., {
range: new Range (...),
...
} );
this. temperatureReadOnlyProperty = this.temperatureProperty;
// constructor and methods can then set this.temperatureProperty
}
}
// client
const thermometer = new Thermometer();
thermometer.temperatureProperty.link( ... ); // ERROR, private
thermometer.temperatureReadOnlyProperty.link( ... ); // OKQuestions?
thermometerProperty and temperatureReadOnlyProperty? thermometerWriteableProperty and temperatureReadOnlyProperty?_temperatureProperty and temperatureProperty?temperatureReadOnlyProperty?samreid
commented
2 years ago I considered 2 strategies for how to allow the class to write to the Property but only allowing clients to read the Property. Strategy A is the one you mentioned above, which seems good to me. An alternative is to only have one field for it, but to provide access via a narrowing getter like so:
Strategy B:
class Thermometer {
// Writeable by this class and its subclasses.
protected readonly _temperatureProperty: NumberProperty;
constructor() {
this._temperatureProperty = new NumberProperty(0);
// constructor and methods can then set this._temperatureProperty
}
get temperatureProperty():ReadOnlyProperty<number>{
return this._temperatureProperty;
}
winterFreeze(){
this._temperatureProperty.set(-100);
}
}
// client
const thermometer = new Thermometer();
thermometer.temperatureProperty.set( ... ); // Type Error, method does not exist in interface
thermometer.temperatureProperty.link( ()=>{} ); // OK
thermometer.winterFreeze(); // OKStrategy A: the duplicate class attribute is not great and the naming is confusing. Strategy B: Requires an underscore and a getter method. Class has to remember the underscored version when changing the value.
Does this require 2 references to the same Property, as in Thermometer? If so, how should they be named?
Let's discuss whether Strategy B is preferable before deciding on the naming for Strategy A.
How does ReadOnlyProperty declaration work with TinyProperty, NumberProperty, etc, as in
I think strategy A or B would work with any Property types or subtypes that match the interface.
New questions:
ReadonlyProperty or two words like ReadOnlyProperty?pixelzoom
commented
2 years ago I like Strategy B, but...
thermometerProperty.rangeProperty? thermometerProperty.rangeProperty is also read-only if I don't want the client to change it? ReadOnlyProperty work for subclasses of Property, many of which contain additional features that still need to be accessed in a read-only API? And are those additional features also read-only? (all of them? some of them? optionally?...)pixelzoom
commented
2 years ago In TypeScript "readonly" is one word. Should we make it one word like ReadonlyProperty or two words like ReadOnlyProperty?
Read-only is a compound adjective. And compound adjectives are typically converted to camel case - e.g. "well-behaved three-legged dog" would be wellBehavedThreeLeggedDog, not wellbehavedThreeleggedDog. So I think that TS has it wrong here. And assuming that we're striving for consistency, there would be other consequences of using "readonly" in names. E.g. renaming phetioReadOnly to phetioReadonly will break all PhET-iO APIs, and maybe some PhET-iO client code.
samreid
commented
2 years ago How does the client get access to thermometerProperty.rangeProperty?
We could make a more general ReadOnly generic like:
type ReadOnly<X, T> =
Omit<X, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
& getValue<T>;Usage would look like:
get temperatureProperty(): ReadOnly<NumberProperty, number> {
return this._temperatureProperty;
}Call site is OK:
thermometer.temperatureProperty.rangeProperty; // OKHow do I ensure that
thermometerProperty.rangePropertyis also read-only if I don't want the client to change it?
Perhaps we would make a way to add more restrictions in the ReadOnly type.
type ReadOnly<X, T, Z extends string> =
Omit<X, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred' | Z>
& getValue<T>;get temperatureProperty(): ReadOnly<NumberProperty, number, 'rangeProperty'> {thermometer.temperatureProperty.rangeProperty; // ERROR: rangeProperty does not exist on the typeIn general, how does ReadOnlyProperty work for subclasses of Property, many of which contain additional features that still need to be accessed in a read-only API?
The proposal to think of ReadOnly as a generic type that works for any subtype of Property seems like it would work well.
Read-only is a compound adjective. And compound adjectives are typically converted to camel case - e.g. "well-behaved three-legged dog" would be wellBehavedThreeLeggedDog, not wellbehavedThreeleggedDog. So I think that TS has it wrong here.
I agree, we should go with ReadOnly.
New Questions:
pixelzoom
commented
2 years ago
- Do you think it is acceptable to add new *.ts code (like this) to common code repos?
In general, yes.
- Is this ReadOnly proposal good enough to commit and start using? ...
This is a feature that will undoubtedly be used widely, and it seems like "type narrowing" is potentially a good direction. But it's difficult to evaluate whether ReadOnlyPoperty is the future without knowing how will axon change when converted to TS. And there are so many problems with Property et. al, some (many?) of which we hope TS fixes. So my intuition tells me that it would be better to wait until we convert the entire Property stack to TS, and have addressed https://github.com/phetsims/axon/issues/343.
samreid
commented
2 years ago The proposal for this issue seems reasonable, but the recommendation is to wait until we have discussed https://github.com/phetsims/axon/issues/343. Going on hold until then.
zepumph
commented
2 years ago This issue has been assigned to me to review for a really long time. I'm unsure if it is still the right place for it, but here is a review of axon files being converted to typescript, which seems to have occurred here. Please let me know if there is more that I should do here, or assign me a side issue if this one is on hold or too long.
Review:
Property,ts
Listener2, or do you have a better idea?valueOf functionNumberProperty
type NumberPropertyOptions = Partial<NumberPropertyDefinedOptions> & Omit<PropertyOptions<number>, 'phetioType'>;I don't have any comments that I can see for DerivedProperty, except that it the Types declared at the top are really awesome. Perhaps as I use it more I will have thoughts, but right now I am mostly impressed with how seamless it feels with the current implementation.
samreid
commented
2 years ago Regarding the review in the preceding comment, as far as I can tell all those issues have been resolved in other work.
We no longer need the generic described in https://github.com/phetsims/axon/issues/363#issuecomment-965535469.
We have discussed Strategy B from https://github.com/phetsims/axon/issues/363#issuecomment-965443445 in dev meeting, it seems useful sometimes.
@zepumph and @pixelzoom can you think of remaining work for this issue? Can it be closed?
zepumph
commented
2 years ago Not for me!
pixelzoom
commented
2 years ago Me neither. Closing.
From https://github.com/phetsims/chipper/issues/1137#issuecomment-961313135 we identified that we would like a way to say that a Property can have its value read and linked but not set.
This may overlap with #343