phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Address `strictAxonDependencies: false` in MeasuringTapeNode #830

Closed pixelzoom closed 8 months ago

pixelzoom commented 8 months ago

For https://github.com/phetsims/axon/issues/441, @samreid added 1 occurrence of strictAxonDependencies: false to MeasuringTapeNode. The missing dependencies should be addressed.

Relevant code in MeasuringTapeNode.ts:

    const readoutStringProperty = new DerivedStringProperty(
      [ this.unitsProperty, this.measuredDistanceProperty, SceneryPhetStrings.measuringTapeReadoutPatternStringProperty ],
      ( units, measuredDistance, measuringTapeReadoutPattern ) => {
        const distance = Utils.toFixed( units.multiplier * measuredDistance, this.significantFigures );
        return StringUtils.fillIn( measuringTapeReadoutPattern, {
          distance: distance,
          units: units.name
        } );
      }, {
        tandem: options.phetioReadoutStringPropertyInstrumented ? options.tandem?.createTandem( 'readoutStringProperty' ) : Tandem.OPT_OUT,
        phetioDocumentation: 'The text content of the readout on the measuring tape',
        strictAxonDependencies: false //TODO https://github.com/phetsims/scenery-phet/issues/830
      } );
pixelzoom commented 8 months ago

Over in https://github.com/phetsims/faradays-electromagnetic-lab/issues/57#issuecomment-1910945878, we turned off strictAxonDependencies when changing listener order via ?listenerOrder.

Next step for this issue is to remove strictAxonDependencies: false from MeasuringTapeNode.ts, then see where it fails in CT. If it fails in a test related to ?listenerOrder, then this issue is now irrelevant and can be closed. If it fails with default listener order, then there's more work to do.

pixelzoom commented 8 months ago

Energy Skate Park had a buggy use of MeasuringTapeNode, would have been caught if the code was in TypeScript, fixed in https://github.com/phetsims/energy-skate-park/commit/d0952ae7cd0003a2c3e768f6d56f4fbf9ac6d0b8. I suspect that may have been the cause of the dependency problem.

So I removed strictAxonDependencies: false from MeasuringTapeNode, then fuzz tested all sims that use MeasuringTapeNode, and hit no failures.

I'll leave this open to verify that CT is happy, and that it's not failing in some other CT test that I didn't run locally.

pixelzoom commented 8 months ago

CT is happy. Closing.