phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

Eliminate Validation.valueType and Validation.arrayElementType #396

Closed samreid closed 2 years ago

samreid commented 2 years ago

As noted in https://github.com/phetsims/studio/issues/253#issuecomment-1097511671, those parts are now checked by the type checker and not necessary for our validation engine. This will prune 2 of the 7 current Validation keys.

@zepumph and @pixelzoom sound reasonable as a long-term goal? (May be difficult to implement). Or are there enough cases where we will wish we kept it. Maybe after loading an untyped state from a JSON file, we would want to check a value type using this?

zepumph commented 2 years ago

Type checking across the PhET-iO featureset is really the only thing that worries me here. Let's really dive into what it is doing for us so we can see if it is valuable.

An IOType has a validator that is used in cases like when a Property value is set, when an emitter emits arguments, and other sim internal items. This in general seems like it is most likely now superfluous checking.

An IOType is also used to validate the return types of fromStateObject. This mostly doesn't seem that important since we are in control when writing the return types.

A Client sends a command with a serialized version of an instance, like { value: 5 } instead of a NumberProperty instance with a value of 5. Then that is passed to the PhetioCommandProcessor where it has IOType.fromStateObject deserialize that before using it in PhET sims.

Is it valuable to have a valueType validator to validate that fromStateObject successfully deserialized into the right instance? In the vast majority of cases probably not. Still though there is some concern about having fromStateObject receive code that has never been run by our codebase before, and not having adequate type checking at runtime to confirm valid deserialization.

P.S Whenever I said fromStateObject, it also applies to applyState, just likely to composite memebers, like how we call RangeIO.fromStateObject in NumberProperty in applyState.

@samreid, I know we have talked about this a few times, and I have always sounded reserved. My main feelings are that I don't want to be bitten by this in the future, and I want to feel confident that we are doing what we can to create intelligent, eager, and helpful assertions to communicate when the user did something wrong. Can you respond to this and help finish off convincing me that we should do this issue?

pixelzoom commented 2 years ago

Validation.arrayElementType looks like it only has a few usages, so might be reasonable to elminate in the near term.

Validation.valueType has many uses, not just for PhET-iO, and not just for the valueType option. And TypeScript isn't going to make these usages redundant until the code is converted to TypeScript. So examples like the Natural Selection usage mentioned in https://github.com/phetsims/studio/issues/253#issuecomment-1097511671 still serve a purpose. And I'll be really surprised if Natural Selection gets converted to TypeScript within the next 2 years. So my vote would be to keep Validation.valueType for now, and reevaluate in a couple of years.

samreid commented 2 years ago

I eliminated arrayElementType and validator got a lot simpler. There were only a few usage sites, which converted to isValidValue easily. Perhaps someday in the future when more sims are TS we may have less of a need for valueType, but this issue seems good to close.