Closed pixelzoom closed 1 year ago
This problem is widespread. For example, looking only at IOType definitions:
@samreid I see this anti-pattern throughout cck-common:
@jonathanolson in density-buoyancy-common and gravity-and-orbits:
@jbphet in greenhouse-effect, you're instantiating these inside the class definition, but missing readonly
modifier:
public static
in Photon.ts@marlitas in mean-share-and-balance:
@jonathanolson in scenery:
Tried out the Vector2 example (since it was not ported that way because of ImmutableVector2), and it fails:
I don't see a way of handling this with other ways easily, can you recommend a pattern for use at this site other than what's being done?
Avoiding this pattern for most of the above cases however seems quite helpful.
Addressed the MSaB occurrence. This seems useful and writing a lint rule is a good idea.
@marlitas Static constants should not be assigned in the constructor, because the constructor is called every time that new Pipe
is called. See the commit above for the correct way to define a static constant. Let me know if you have any questions.
I agree these are antipatterns but also wanted to point out the overlap in https://github.com/phetsims/tandem/issues/275 in which we may (as a long term solution) combine IOTypes with the core types.
Yes, there's indeed overlap with #275. But I want to emphazied that, while the examples I've pointed to involve IOTypes, the antipattern applies to all statics, not just IOTypes. Do a search for "public static" and you'll find many examples.
Thanks for pointing this out @pixelzoom. I made changes in quite a few places in greenhouse-effect, beyond the list shown above. @marlitas - I agree that a lint rule for this would be helpful.
Here is the example AST, it should be easy to make a lint rule to flag when readonly
is missing: https://typescript-eslint.io/play/#ts=4.8.4&sourceType=module&showAST=es&code=MYGwhgzhAECyCeBhcVoG8CwAoa0AOArgEYgCWw0EALmFedAE4CmYAJgPYB2I80AKgFEAynwD6fABIBJAHIBxaAF5oAckEiVAbmwBfIA
Might also be easy to add an auto-fix.
EDIT: Well... easy to find these in the AST but not easy to determine the intent, lots of public static
should probably still be writable.
Dev meeting Nov 17 2022.
We agreed that making public static class properties readonly
is best. Just something that needs to be cleaned up.
@zepumph: I see 262 usages where this pattern is happening. Maybe half of them can be fixed. So that is maybe 2 hours of work to do it all.
@AgustinVallejo: I'm happy to help on this.
@kathy-phet: Can @pixelzoom and @AgustinVallejo pair together for a "kick-off" meeting, then @AgustinVallejo take it from there?
@pixelzoom & @AgustinVallejo: Great!
Here is a regex from @zepumph that may help static .*:.*;
@AgustinVallejo and I met about this after today's dev meeting. He'll use the regex static .*:.*;
to identify potential places to add readonly
. For any that prove to be problematic (like Vector2), he'll note them, and he and I will review.
Regex static .*:.*;
is not identifying all cases. We also need to check static .*=.*
(assignment) and static .*:.*
(mulit-line).
Here are some weird cases we found regarding this issue, we'll be looking into it.
Decided to leave it as it is because of class dependencies:
Other:
In the above commits, I addressed a bunch more cases. There are still some cases that have not been addressed (as indicated in https://github.com/phetsims/tandem/issues/278#issuecomment-1334098347) because they are problematic. And there are some cases in sun and scenery that are just a little too much work to untangle and test. But I don't feel like we need to address every case for this issue (diminishing returns...), and I feel like we're at a good stopping point. @AgustinVallejo do you agree? If so, feel free to close this issue. If not, what else should we do?
I'm seeing a pattern of definiting static class properties that is concerning. It results in those constants being writeable when they should be
readonly
.For example, here's Vector2.ts:
All of the
static
properties should bestatic readonly
. And they should be instantiated inside theclass
definition, not outside the class definition. Like this:A couple of things for discussion at developer meeting:
(1) PSA and recommendation not to use this pattern. Statics that are read-only should have the
readonly
modifier.(2) Can we write a lint rule to prevent this, and identify all of the existing cases?