Skip to content

Commit

Permalink
Remove strictAxonDependencies feature, see #441
Browse files Browse the repository at this point in the history
  • Loading branch information
samreid committed Jun 13, 2024
1 parent 77901f7 commit f70c7c5
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 47 deletions.
36 changes: 6 additions & 30 deletions js/DerivedProperty.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,45 +19,27 @@ import TReadOnlyProperty from './TReadOnlyProperty.js';
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import optionize from '../../phet-core/js/optionize.js';
import { Dependencies, RP1, RP10, RP11, RP12, RP13, RP14, RP15, RP2, RP3, RP4, RP5, RP6, RP7, RP8, RP9 } from './Multilink.js';
import ReadOnlyProperty, { derivationStack } from './ReadOnlyProperty.js';
import ReadOnlyProperty from './ReadOnlyProperty.js';
import PhetioObject from '../../tandem/js/PhetioObject.js';
import IOTypeCache from '../../tandem/js/IOTypeCache.js';
import TinyStaticProperty from './TinyStaticProperty.js';

const DERIVED_PROPERTY_IO_PREFIX = 'DerivedPropertyIO';

type SelfOptions = {

// When true, if this DerivedProperty is PhET-iO instrument, add a LinkedElement for each PhET-iO instrumented dependency.
phetioLinkDependencies?: boolean;

// Typically, a DerivedProperty should only get values for its immediate dependencies, otherwise it could end up in a
// situation where some value central to the derivation has changed, but the derived property doesn't update because
// it isn't listening. These checks can be enabled via a package.json flag simFeatures.strictAxonDependencies or
// the query parameter ?strictAxonDependencies=true. The query parameter takes precedence.
// See https://github.com/phetsims/axon/issues/441
strictAxonDependencies?: boolean;
};

export type DerivedPropertyOptions<T> = SelfOptions & PropertyOptions<T>;

const strictAxonDependenciesGlobal = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.strictAxonDependencies;

/**
* Compute the derived value given a derivation and an array of dependencies
*/
function getDerivedValue<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>( strictAxonDependencies: boolean, derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15> ): T {

assert && strictAxonDependenciesGlobal && strictAxonDependencies && derivationStack.push( dependencies );

try {
function getDerivedValue<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>( derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15> ): T {

// @ts-expect-error
return derivation( ...dependencies.map( property => property.get() ) );
}
finally {
assert && strictAxonDependenciesGlobal && strictAxonDependencies && derivationStack.pop();
}
}

// Convenience type for a Derived property that has a known return type but unknown dependency types.
Expand All @@ -71,7 +53,6 @@ export default class DerivedProperty<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10,
private dependencies: Dependencies<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15> | null;
private readonly derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T;
private readonly derivedPropertyListener: () => void;
private readonly strictAxonDependencies: boolean;

public static DerivedPropertyIO: ( parameterType: IOType ) => IOType;

Expand Down Expand Up @@ -101,17 +82,13 @@ export default class DerivedProperty<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10,
const options = optionize<DerivedPropertyOptions<T>, SelfOptions, PropertyOptions<T>>()( {
phetioReadOnly: true, // derived properties can be read but not set by PhET-iO
phetioOuterType: DerivedProperty.DerivedPropertyIO,
phetioLinkDependencies: true,

// TinyStaticProperties are only used in Node for supporting layout. This common code does not support knowing
// all dependencies at creation time. See https://github.com/phetsims/faradays-electromagnetic-lab/issues/171
strictAxonDependencies: !_.some( dependencies, dependency => dependency instanceof TinyStaticProperty )
phetioLinkDependencies: true
}, providedOptions );

assert && assert( dependencies.every( _.identity ), 'dependencies should all be truthy' );
assert && assert( dependencies.length === _.uniq( dependencies ).length, 'duplicate dependencies' );

const initialValue = getDerivedValue( options.strictAxonDependencies, derivation, dependencies );
const initialValue = getDerivedValue( derivation, dependencies );

// We must pass supertype tandem to parent class so addInstance is called only once in the subclassiest constructor.
super( initialValue, options );
Expand All @@ -124,7 +101,6 @@ export default class DerivedProperty<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10,

this.dependencies = dependencies;
this.derivation = derivation;
this.strictAxonDependencies = options.strictAxonDependencies;
this.derivedPropertyListener = this.getDerivedPropertyListener.bind( this );

dependencies.forEach( dependency => {
Expand Down Expand Up @@ -180,7 +156,7 @@ export default class DerivedProperty<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10,
this.hasDeferredValue = true;
}
else {
super.set( getDerivedValue( this.strictAxonDependencies, this.derivation, this.definedDependencies ) );
super.set( getDerivedValue( this.derivation, this.definedDependencies ) );
}
}

Expand Down Expand Up @@ -218,7 +194,7 @@ export default class DerivedProperty<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10,
*/
public override setDeferred( isDeferred: boolean ): ( () => void ) | null {
if ( this.isDeferred && !isDeferred ) {
this.deferredValue = getDerivedValue( this.strictAxonDependencies, this.derivation, this.definedDependencies );
this.deferredValue = getDerivedValue( this.derivation, this.definedDependencies );
}
return super.setDeferred( isDeferred );
}
Expand Down
17 changes: 0 additions & 17 deletions js/ReadOnlyProperty.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
};

export const derivationStack: Array<IntentionalAny> = [];

/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
* interface. Initial value and resetting is not defined here.
Expand Down Expand Up @@ -242,21 +240,6 @@ export default class ReadOnlyProperty<T> extends PhetioObject implements TReadOn
* or internal code that must be fast.
*/
public get(): T {

if ( assert ) {

// When changing the listener order via ?listenerOrder, we were running into strictAxonDependencies failures that
// did not otherwise occur. Because we aren't interested in these corner cases, and because they are difficult to
// understand and debug, we chose to turn off strictAxonDependencies if listener order is changed.
// See https://github.com/phetsims/faradays-electromagnetic-lab/issues/57#issuecomment-1909089735
const strictAxonDependencies = _.hasIn( window, 'phet.chipper.queryParameters' ) &&
phet.chipper.queryParameters.strictAxonDependencies &&
phet.chipper.queryParameters.listenerOrder === 'default';
if ( strictAxonDependencies && derivationStack.length > 0 ) {
const currentDependencies = derivationStack[ derivationStack.length - 1 ];
assert && assert( currentDependencies.includes( this ), 'accessed value outside of dependency tracking' );
}
}
return this.tinyProperty.get();
}

Expand Down

0 comments on commit f70c7c5

Please sign in to comment.