Should validate be moved out of axon?

jessegreenberg commented 3 years ago

In https://github.com/phetsims/utterance-queue/issues/33 I tried to use validate in a new function in phet-core. But phet-core shouldn't have a dependency on axon because axon already depends on phet-core.

I am wondering if validate and ValidatorDef should be moved to phet-core (or maybe somewhere else) as its usage does not seem limited to observables and axon things.

Assigning to @samreid and @zepumph for thoughts.

zepumph commented 3 years ago

Yes, this should move to phet-core, but I really want to keep the history. Can we please use copy-history-to-different-repo.sh to move over validate.js, ValidatorDef.js, and ValidatorDefTests.js?

samreid commented 3 years ago

ValidatorDefTests imports from both scenery/ and tandem/. Do we really want to put a dependency from phet-core to scenery and tandem? How else can this be done? Should we drop the Node parts of the tests, and use EnumerationIO instead of StringIO in the tests?

jessegreenberg commented 3 years ago

Are those tests in ValidatorDefTests specific to ValidatorDef or could they be moved to other tests? For example could the 'Test phetioType' portion be moved to IOTypeTests.js or something else?

zepumph commented 3 years ago

A couple of things here.

EnumerationIO is in phet-core and uses tandem.
All the cross dependencies are arbitrary, and just there to test types. They could easily be rewritten to just use IOTypes in phet-core.
In general, I thought that tests don't have the same constraints on dependencies than other code. Is that not true? If that has changed for typescript, or if I never understood to begin with, then do we need to discuss this at dev meeting?
Let me know if you want me to do that refactor for test dependencies.

samreid commented 3 years ago

EnumerationIO is in phet-core and uses tandem.

Yes, this creates complications for TypeScript. Note that EnumerationIO is excluded from phet-core/tsconfig.json, and included over in tandem/tsconfig.json.

All the cross dependencies are arbitrary, and just there to test types. They could easily be rewritten to just use IOTypes in phet-core.

That seems like a good move.

In general, I thought that tests don't have the same constraints on dependencies than other code. Is that not true? If that has changed for typescript, or if I never understood to begin with, then do we need to discuss this at dev meeting?

I started to observe these problems in https://github.com/phetsims/chipper/issues/1052 and made progress on https://github.com/phetsims/chipper/issues/1055. To me, it adds complexity that, say, phet-core imports files from scenery. I'm not aware of any prior problems with that, but it has created wrinkles for TypeScript.

Let me know if you want me to do that refactor for test dependencies.

That would be great, thanks! I can also volunteer, but am very backlogged at the moment.

zepumph commented 3 years ago

I will try hard to get to the grunt work here soon to get this off hold for https://github.com/phetsims/utterance-queue/issues/33

zepumph commented 3 years ago

Yes, this creates complications for TypeScript. Note that EnumerationIO is excluded from phet-core/tsconfig.json, and included over in tandem/tsconfig.json.

@samreid, would you like me to make an issue to move IO types in phet-core out to tandem? Perhaps a lint rule in phet-core to make sure that it doesn't have any cross dependencies?

samreid commented 3 years ago

Is EnumerationIO the only IO type in phet-core? Would we want to move it out but leave Enumeration, EnumerationMap and EnumerationTests behind? What is the likelihood Enumeration will one day need to import EnumerationIO?

zepumph commented 3 years ago

What is the likelihood Enumeration will one day need to import EnumerationIO?

Very high in my opinion. Most likely it should be in there now. Similar to the way we do other data types like Vector2IO.

ValidatorDef as a feature does depend on tandem though, even if the tests don't reflect that. This is because a validator can have a phetioType provided. This is of type IOType, even if we don't include that check in code that lives in phet-core. I think I would rather have a cross dependency, than for that to be behind the scenes.

Currently it is just duck typed:

https://github.com/phetsims/axon/blob/201a8de0069ed073d90e2b580254daa97486fc75/js/ValidatorDef.js#L158-L164

But I don't think that is as good as if there was an instanceof check for IOType.

I don't see these relationships as hierarchical, so I don't really know how to proceed. We could create some sort of central library of phet that DOES depend on tandem, and keep phet-core as something like has no dependencies. Then Enumeration would also go into the other library.

zepumph commented 3 years ago

This needs to be solved for Enumeration and tandem at the same time. BTW we are no longer blocking https://github.com/phetsims/utterance-queue/issues/33, as we found a way around it.

samreid commented 3 years ago

After our discussion today, it's unclear to me what the options are, or what else we need to discuss/decide before we can proceed. @zepumph can you please advise?

zepumph commented 2 years ago

This does not need to need to occur for any issue that this was originally brought up for. While it doesn't totally make sense for this to be in axon, we don't currently have a better spot for it that doesn't create another headache for cross dependencies. Let's reopen if we find this to dependency tree to be too burdensome. Closing

phetsims / axon

Should validate be moved out of axon? #361