Node subtypes mutate function not allowing subtype-specific options #1428
Assignees
Comments
|
Pining here as this is was one I ran into over in phetsims/chipper#1366 (comment) I believe the solution is to override mutate just to change the parameter signature. Not sure if that is complete or if there is a less annoying way to solve this though. @jonathanolson let me know if you want help! |
34 tasks
If we want to satisfy the liskov substitution principle, then subclass parameter types can only become more general, not more specific (as it is in the contravariant position). Are we trying to keep Node non-parametric? If not, we could make: Node<TNodeOptions=NodeOptions>{
mutate(options: TNodeOptions){}
}For example, this patch solves the ts-expect-errors in bamboo, while only creating 49 new errors: Subject: [PATCH] test
---
Index: main/bamboo/js/UpDownArrowPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bamboo/js/UpDownArrowPlot.ts b/main/bamboo/js/UpDownArrowPlot.ts
--- a/main/bamboo/js/UpDownArrowPlot.ts (revision 68fec7ef1c0a909c1d47351d6d866c490fcb9cec)
+++ b/main/bamboo/js/UpDownArrowPlot.ts (date 1671063630284)
@@ -108,7 +108,6 @@
'options contain keys that could be dangerous for mutate'
);
- // @ts-expect-error, see https://github.com/phetsims/scenery/issues/1428
this.arrowNodes[ i ].mutate( providedOptions );
}
}
Index: main/scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Node.ts b/main/scenery/js/nodes/Node.ts
--- a/main/scenery/js/nodes/Node.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Node.ts (date 1671063755066)
@@ -375,7 +375,7 @@
imageOptions?: ImageOptions;
};
-class Node extends ParallelDOM {
+class Node<TNodeOptions = NodeOptions> extends ParallelDOM {
// NOTE: All member properties with names starting with '_' are assumed to be private/protected!
// Assigns a unique ID to this Node (allows trails to get a unique list of IDs)
@@ -720,7 +720,7 @@
* instead of setting the setter (node.scale = ..., which would override the function), it will instead call
* the method directly (e.g. node.scale( ... )).
*/
- public constructor( options?: NodeOptions ) {
+ public constructor( options?: TNodeOptions ) {
super();
@@ -6243,7 +6243,7 @@
* Additionally, some keys are actually direct function names, like 'scale'. mutate( { scale: 2 } ) will call
* node.scale( 2 ) instead of activating an ES5 setter directly.
*/
- public mutate( options?: NodeOptions ): this {
+ public mutate( options?: TNodeOptions ): this {
if ( !options ) {
return this;
@@ -6300,7 +6300,7 @@
return this; // allow chaining
}
- protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: NodeOptions ): void {
+ protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: TNodeOptions ): void {
// Track this, so we only override our visibleProperty once.
const wasInstrumented = this.isPhetioInstrumented();
Index: main/scenery/js/nodes/Path.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Path.ts b/main/scenery/js/nodes/Path.ts
--- a/main/scenery/js/nodes/Path.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Path.ts (date 1671063755074)
@@ -32,7 +32,7 @@
type ParentOptions = PaintableOptions & NodeOptions;
export type PathOptions = SelfOptions & ParentOptions;
-export default class Path extends Paintable( Node ) {
+export default class Path extends Paintable( Node<PathOptions> ) {
// The Shape used for displaying this Path.
// NOTE: _shape can be lazily constructed in subtypes (may be null) if hasShape() is overridden to return true,
|
|
Changing it to // add all the nodes to mediumPanelNode
const mediumPanelNode = new Node( {
something: true,
children: [ materialComboBox, indexOfRefractionNode, indexOfRefractionSlider, unknown ]
} );Here is that patch though, in case we can get that working. I wouldn't want to have to say Subject: [PATCH] test
---
Index: main/bamboo/js/UpDownArrowPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bamboo/js/UpDownArrowPlot.ts b/main/bamboo/js/UpDownArrowPlot.ts
--- a/main/bamboo/js/UpDownArrowPlot.ts (revision 68fec7ef1c0a909c1d47351d6d866c490fcb9cec)
+++ b/main/bamboo/js/UpDownArrowPlot.ts (date 1671063630284)
@@ -108,7 +108,6 @@
'options contain keys that could be dangerous for mutate'
);
- // @ts-expect-error, see https://github.com/phetsims/scenery/issues/1428
this.arrowNodes[ i ].mutate( providedOptions );
}
}
Index: main/scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Node.ts b/main/scenery/js/nodes/Node.ts
--- a/main/scenery/js/nodes/Node.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Node.ts (date 1671065519640)
@@ -375,7 +375,7 @@
imageOptions?: ImageOptions;
};
-class Node extends ParallelDOM {
+class Node<TNodeOptions extends NodeOptions = NodeOptions> extends ParallelDOM {
// NOTE: All member properties with names starting with '_' are assumed to be private/protected!
// Assigns a unique ID to this Node (allows trails to get a unique list of IDs)
@@ -720,7 +720,7 @@
* instead of setting the setter (node.scale = ..., which would override the function), it will instead call
* the method directly (e.g. node.scale( ... )).
*/
- public constructor( options?: NodeOptions ) {
+ public constructor( options?: TNodeOptions ) {
super();
@@ -5022,7 +5022,7 @@
const trail = new Trail();
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
while ( node ) {
assert && assert( node._parents.length <= 1,
@@ -5987,7 +5987,7 @@
*/
public getLocalToGlobalMatrix(): Matrix3 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<TNodeOptions> = this; // eslint-disable-line consistent-this
// we need to apply the transformations in the reverse order, so we temporarily store them
const matrices = [];
@@ -6041,7 +6041,7 @@
public localToGlobalPoint( point: Vector2 ): Vector2 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
const resultPoint = point.copy();
while ( node ) {
// in-place multiplication
@@ -6061,7 +6061,7 @@
public globalToLocalPoint( point: Vector2 ): Vector2 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
// TODO: performance: test whether it is faster to get a total transform and then invert (won't compute individual inverses)
// we need to apply the transformations in the reverse order, so we temporarily store them
@@ -6243,7 +6243,7 @@
* Additionally, some keys are actually direct function names, like 'scale'. mutate( { scale: 2 } ) will call
* node.scale( 2 ) instead of activating an ES5 setter directly.
*/
- public mutate( options?: NodeOptions ): this {
+ public mutate( options?: TNodeOptions ): this {
if ( !options ) {
return this;
@@ -6300,7 +6300,7 @@
return this; // allow chaining
}
- protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: NodeOptions ): void {
+ protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: TNodeOptions ): void {
// Track this, so we only override our visibleProperty once.
const wasInstrumented = this.isPhetioInstrumented();
Index: main/scenery/js/nodes/Path.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Path.ts b/main/scenery/js/nodes/Path.ts
--- a/main/scenery/js/nodes/Path.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Path.ts (date 1671063755074)
@@ -32,7 +32,7 @@
type ParentOptions = PaintableOptions & NodeOptions;
export type PathOptions = SelfOptions & ParentOptions;
-export default class Path extends Paintable( Node ) {
+export default class Path extends Paintable( Node<PathOptions> ) {
// The Shape used for displaying this Path.
// NOTE: _shape can be lazily constructed in subtypes (may be null) if hasShape() is overridden to return true,
Index: main/bamboo/js/BarPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bamboo/js/BarPlot.ts b/main/bamboo/js/BarPlot.ts
--- a/main/bamboo/js/BarPlot.ts (revision 68fec7ef1c0a909c1d47351d6d866c490fcb9cec)
+++ b/main/bamboo/js/BarPlot.ts (date 1671065706324)
@@ -105,7 +105,6 @@
'options contain keys that could be dangerous for mutate'
);
- // @ts-expect-error - mutate needs to know about the suboptions, see https://github.com/phetsims/scenery/issues/1428
this.rectangles[ i ].mutate( paintableFields );
}
}
Index: main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts b/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts
--- a/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts (revision 8eae01ea4d7777da02408f5baace273de460fbaa)
+++ b/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts (date 1671065706336)
@@ -136,7 +136,6 @@
const paintableFields: PaintableOptions = {
fill: new Color( 'fuchsia' ).darkerColor( area.completion )
};
- // @ts-expect-error - mutate needs to know about the suboptions, see https://github.com/phetsims/scenery/issues/1428
barPlot.rectangles[ index ].mutate( paintableFields );
} );
};
Index: main/bending-light/js/common/view/MediumControlPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bending-light/js/common/view/MediumControlPanel.ts b/main/bending-light/js/common/view/MediumControlPanel.ts
--- a/main/bending-light/js/common/view/MediumControlPanel.ts (revision dbaf3e5dc6231a0ec28fbc742e2b6889abf3bc7e)
+++ b/main/bending-light/js/common/view/MediumControlPanel.ts (date 1671065706328)
@@ -312,9 +312,8 @@
// add all the nodes to mediumPanelNode
const mediumPanelNode = new Node( {
- children: [ materialComboBox, indexOfRefractionNode, indexOfRefractionSlider, unknown ],
- // @ts-expect-error TODO: Spacing isn't on Node
- spacing: 10
+ something: true,
+ children: [ materialComboBox, indexOfRefractionNode, indexOfRefractionSlider, unknown ]
} );
const mediumPanel = new Panel( mediumPanelNode, {
Index: main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts b/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts
--- a/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts (revision 13048a728d780aad982b483389ccd17344d665ed)
+++ b/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts (date 1671065706320)
@@ -10,7 +10,7 @@
import BooleanProperty from '../../../axon/js/BooleanProperty.js';
import NumberProperty from '../../../axon/js/NumberProperty.js';
import Property from '../../../axon/js/Property.js';
-import { AlignGroup, Color, Image, Line, Node } from '../../../scenery/js/imports.js';
+import { AlignGroup, Color, Image, Line, Node, NodeOptions } from '../../../scenery/js/imports.js';
import Vector2 from '../../../dot/js/Vector2.js';
import ToggleNode from '../../../sun/js/ToggleNode.js';
import Tandem from '../../../tandem/js/Tandem.js';
@@ -145,7 +145,7 @@
}, providedOptions );
const wrap = ( node: Node, height: number ) => {
- const node1 = new Node( {
+ const node1 = new Node<NodeOptions>( {
children: [ node ]
} );
node1.mutate( { scale: height / node1.height } );
Index: main/circuit-construction-kit-common/js/view/FuseNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/view/FuseNode.ts b/main/circuit-construction-kit-common/js/view/FuseNode.ts
--- a/main/circuit-construction-kit-common/js/view/FuseNode.ts (revision 13048a728d780aad982b483389ccd17344d665ed)
+++ b/main/circuit-construction-kit-common/js/view/FuseNode.ts (date 1671065706332)
@@ -11,7 +11,7 @@
import Utils from '../../../dot/js/Utils.js';
import Vector2 from '../../../dot/js/Vector2.js';
import { Shape } from '../../../kite/js/imports.js';
-import { Color, Image, Node, Path, Rectangle } from '../../../scenery/js/imports.js';
+import { Color, Image, Node, NodeOptions, Path, Rectangle } from '../../../scenery/js/imports.js';
import Tandem from '../../../tandem/js/Tandem.js';
import fuse_png from '../../images/fuse_png.js';
import CCKCConstants from '../CCKCConstants.js';
@@ -99,7 +99,7 @@
lineWidth: 0.5
} );
- const lifelikeFuseNode = new Node( {
+ const lifelikeFuseNode = new Node<NodeOptions>( {
children: [ filamentPath, glassNode, fuseImageNode ]
} );
@zepumph what do you think? |
This made me sad to read, but then I went to your patch and applied it and I see it error. Yay?!???
UPDATE: I feel like my webstorm just hadn't caught up yet. This behavior is not true. You are right. It isn't throwing a type error. |
|
I had some luck getting better type safety when omitting the |
|
I thought of a way to avoid inference in Node construction by making the constructor options non-parametric. I updated a few spots in the patch and it seems to be working ok. @zepumph can you please review? Subject: [PATCH] Support null or undefined for bondLengthOverride, see https://github.com/phetsims/molecule-shapes/issues/233
---
Index: main/bamboo/js/UpDownArrowPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bamboo/js/UpDownArrowPlot.ts b/main/bamboo/js/UpDownArrowPlot.ts
--- a/main/bamboo/js/UpDownArrowPlot.ts (revision 68fec7ef1c0a909c1d47351d6d866c490fcb9cec)
+++ b/main/bamboo/js/UpDownArrowPlot.ts (date 1671229923915)
@@ -108,7 +108,6 @@
'options contain keys that could be dangerous for mutate'
);
- // @ts-expect-error, see https://github.com/phetsims/scenery/issues/1428
this.arrowNodes[ i ].mutate( providedOptions );
}
}
Index: main/scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Node.ts b/main/scenery/js/nodes/Node.ts
--- a/main/scenery/js/nodes/Node.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Node.ts (date 1671230148450)
@@ -375,7 +375,7 @@
imageOptions?: ImageOptions;
};
-class Node extends ParallelDOM {
+class Node<TNodeOptions extends NodeOptions = NodeOptions> extends ParallelDOM {
// NOTE: All member properties with names starting with '_' are assumed to be private/protected!
// Assigns a unique ID to this Node (allows trails to get a unique list of IDs)
@@ -5022,7 +5022,7 @@
const trail = new Trail();
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
while ( node ) {
assert && assert( node._parents.length <= 1,
@@ -5987,7 +5987,7 @@
*/
public getLocalToGlobalMatrix(): Matrix3 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<TNodeOptions> = this; // eslint-disable-line consistent-this
// we need to apply the transformations in the reverse order, so we temporarily store them
const matrices = [];
@@ -6041,7 +6041,7 @@
public localToGlobalPoint( point: Vector2 ): Vector2 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
const resultPoint = point.copy();
while ( node ) {
// in-place multiplication
@@ -6061,7 +6061,7 @@
public globalToLocalPoint( point: Vector2 ): Vector2 {
// eslint-disable-next-line @typescript-eslint/no-this-alias
- let node: Node = this; // eslint-disable-line consistent-this
+ let node: Node<IntentionalAny> = this; // eslint-disable-line consistent-this
// TODO: performance: test whether it is faster to get a total transform and then invert (won't compute individual inverses)
// we need to apply the transformations in the reverse order, so we temporarily store them
@@ -6243,7 +6243,7 @@
* Additionally, some keys are actually direct function names, like 'scale'. mutate( { scale: 2 } ) will call
* node.scale( 2 ) instead of activating an ES5 setter directly.
*/
- public mutate( options?: NodeOptions ): this {
+ public mutate( options?: TNodeOptions | NodeOptions ): this {
if ( !options ) {
return this;
@@ -6300,7 +6300,7 @@
return this; // allow chaining
}
- protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: NodeOptions ): void {
+ protected override initializePhetioObject( baseOptions: Partial<PhetioObjectOptions>, config: TNodeOptions | NodeOptions ): void {
// Track this, so we only override our visibleProperty once.
const wasInstrumented = this.isPhetioInstrumented();
Index: main/scenery/js/nodes/Path.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Path.ts b/main/scenery/js/nodes/Path.ts
--- a/main/scenery/js/nodes/Path.ts (revision 6e0a6e83487bd43159a97073dad13291764cb7c0)
+++ b/main/scenery/js/nodes/Path.ts (date 1671229923999)
@@ -32,7 +32,7 @@
type ParentOptions = PaintableOptions & NodeOptions;
export type PathOptions = SelfOptions & ParentOptions;
-export default class Path extends Paintable( Node ) {
+export default class Path extends Paintable( Node<PathOptions> ) {
// The Shape used for displaying this Path.
// NOTE: _shape can be lazily constructed in subtypes (may be null) if hasShape() is overridden to return true,
Index: main/bamboo/js/BarPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bamboo/js/BarPlot.ts b/main/bamboo/js/BarPlot.ts
--- a/main/bamboo/js/BarPlot.ts (revision 68fec7ef1c0a909c1d47351d6d866c490fcb9cec)
+++ b/main/bamboo/js/BarPlot.ts (date 1671229923910)
@@ -105,7 +105,6 @@
'options contain keys that could be dangerous for mutate'
);
- // @ts-expect-error - mutate needs to know about the suboptions, see https://github.com/phetsims/scenery/issues/1428
this.rectangles[ i ].mutate( paintableFields );
}
}
Index: main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts b/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts
--- a/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts (revision 38e528d87b0a1e789ef5df7c7a5521e95888025d)
+++ b/main/my-solar-system/js/keplers-laws/view/AreasGraphPanel.ts (date 1671229923933)
@@ -136,7 +136,6 @@
const paintableFields: PaintableOptions = {
fill: new Color( 'fuchsia' ).darkerColor( area.completion )
};
- // @ts-expect-error - mutate needs to know about the suboptions, see https://github.com/phetsims/scenery/issues/1428
barPlot.rectangles[ index ].mutate( paintableFields );
} );
};
Index: main/bending-light/js/common/view/MediumControlPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bending-light/js/common/view/MediumControlPanel.ts b/main/bending-light/js/common/view/MediumControlPanel.ts
--- a/main/bending-light/js/common/view/MediumControlPanel.ts (revision dbaf3e5dc6231a0ec28fbc742e2b6889abf3bc7e)
+++ b/main/bending-light/js/common/view/MediumControlPanel.ts (date 1671229923920)
@@ -312,9 +312,8 @@
// add all the nodes to mediumPanelNode
const mediumPanelNode = new Node( {
- children: [ materialComboBox, indexOfRefractionNode, indexOfRefractionSlider, unknown ],
- // @ts-expect-error TODO: Spacing isn't on Node
- spacing: 10
+ something: true,
+ children: [ materialComboBox, indexOfRefractionNode, indexOfRefractionSlider, unknown ]
} );
const mediumPanel = new Panel( mediumPanelNode, {
Index: main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts b/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts
--- a/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts (revision ae0e041ccf799376c85731ed74a12a91cd1776bb)
+++ b/main/circuit-construction-kit-common/js/view/CircuitElementToolFactory.ts (date 1671229923926)
@@ -10,7 +10,7 @@
import BooleanProperty from '../../../axon/js/BooleanProperty.js';
import NumberProperty from '../../../axon/js/NumberProperty.js';
import Property from '../../../axon/js/Property.js';
-import { AlignGroup, Color, Image, Line, Node } from '../../../scenery/js/imports.js';
+import { AlignGroup, Color, Image, Line, Node, NodeOptions } from '../../../scenery/js/imports.js';
import Vector2 from '../../../dot/js/Vector2.js';
import ToggleNode from '../../../sun/js/ToggleNode.js';
import Tandem from '../../../tandem/js/Tandem.js';
@@ -145,7 +145,7 @@
}, providedOptions );
const wrap = ( node: Node, height: number ) => {
- const node1 = new Node( {
+ const node1 = new Node<NodeOptions>( {
children: [ node ]
} );
node1.mutate( { scale: height / node1.height } );
Index: main/circuit-construction-kit-common/js/view/FuseNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/view/FuseNode.ts b/main/circuit-construction-kit-common/js/view/FuseNode.ts
--- a/main/circuit-construction-kit-common/js/view/FuseNode.ts (revision ae0e041ccf799376c85731ed74a12a91cd1776bb)
+++ b/main/circuit-construction-kit-common/js/view/FuseNode.ts (date 1671229923929)
@@ -11,7 +11,7 @@
import Utils from '../../../dot/js/Utils.js';
import Vector2 from '../../../dot/js/Vector2.js';
import { Shape } from '../../../kite/js/imports.js';
-import { Color, Image, Node, Path, Rectangle } from '../../../scenery/js/imports.js';
+import { Color, Image, Node, NodeOptions, Path, Rectangle } from '../../../scenery/js/imports.js';
import Tandem from '../../../tandem/js/Tandem.js';
import fuse_png from '../../images/fuse_png.js';
import CCKCConstants from '../CCKCConstants.js';
@@ -99,7 +99,7 @@
lineWidth: 0.5
} );
- const lifelikeFuseNode = new Node( {
+ const lifelikeFuseNode = new Node<NodeOptions>( {
children: [ filamentPath, glassNode, fuseImageNode ]
} );
|
|
Are the changes to FuseNode and CreateCircuitElementToolNode necessary? To me that is a deal breaker to the pattern. We need |
Nope! And in fact, they are forbidden:
|
|
I applied the patch again to try to make some stuff happen, and I immediately ran into a problem with applying this across a whole subtype hierarchy. Take Rectangle for example. It has to pass its RectangleOptions up through Path (past a mixin) and Path has to give that to Node. I tried this in Path.ts, but got an error:
the last usage of TPathOptions says: "TS2562: Base class expressions cannot reference class type parameters." @samreid, any ideas? Part of me still wants to commit your patch, and I think we probably could for some added benefit, but I didn't want to just in case the real and full solution is actually something else. |
|
It seems like the problem is related to mixins only. Since this fails: export default class Path<TPathOptions extends NodeOptions = NodeOptions> extends Paintable( Node<TPathOptions> ) {But this works (no error): export default class Path<TPathOptions extends NodeOptions = NodeOptions> extends Node<TPathOptions> {There is a paper trail starting in |
|
Sorry about the delay in reviewing! While base scenery types try to uphold the "you can mutate with anything you pass to options", this is absolutely NOT true for most of our codebase that extends Node, and it seems like adding a parametric type would likely cause more trouble. Why not e.g. just add the following into the types that add mutator keys?: e.g. in Path: public override mutate( options?: PathOptions ): this {
return super.mutate( options );
} |
|
Also, it looks like this approach has been done for 6 months in RichText, Text and GridBox. |
|
That seems very practical. Maybe the only reason not to use that approach is https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)#Covariant_method_parameter_type that it is not type safe. But maybe our practical concerns are more important? |
jonathanolson
added a commit
to phetsims/keplers-laws
that referenced
this issue
Mar 1, 2023
jonathanolson
added a commit
to phetsims/scenery-phet
that referenced
this issue
Mar 1, 2023
jonathanolson
added a commit
to phetsims/bamboo
that referenced
this issue
Mar 1, 2023
jonathanolson
added a commit
that referenced
this issue
Mar 1, 2023
jonathanolson
added a commit
to phetsims/joist
that referenced
this issue
Mar 1, 2023
|
I'm not concerned about the covariance here... it only technically breaks e.g. if you take an HBox (can't accept orientation), cast it to a FlowBox (can accept orientation), and mutate with an orientation. (weirdly this will work, but gets around our type constraints). For the most common cases, we just add onto options for subtypes, so the typing seems consistent. I've handled this above, and it works really nicely. It's also very compatible with the mixins, e.g. for sizability and a11y, since they just grab the mutate parameter type used for what they're mixing into, and augment it. |
|
@samreid can you review, and see if there are issues with this method that I missed? |
|
Looks great to me. I feel this issue can be closed, but let's check in with @zepumph first. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Discovered while working on phetsims/chipper#1253. Not sure if related to #1423
The problem is that Node subtype
mutatedoesn't seem to have the correct typescript type for its options, so I'm having to put ts-ignores.@jonathanolson can you please advise?
The text was updated successfully, but these errors were encountered: