From f4dbea0a65ad4808f8318f3bbe6aa5000b24c235 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Wed, 25 Jun 2025 04:49:37 -0700 Subject: [PATCH] refactor(interfaces): Make type predicates more robust (#9150) * refactor(interfaces): Use typeof ... === 'function' to test for methods Testing for 'name' in object or obj.name !== undefined only checks for the existence of the property (and in the latter case that the property is not set to undefined). That's fine if the interface specifies a property of indeterminate type, but in the usual case that the interface member is a method we can do one better and check to make sure the property's value is callable. * refactor(interfaces): Always check obj is not null/undefined Since most type predicates take an argument of type any but then check for the existence of certain properties, explicitly check that the argument is not null or undefined (or check implicitly by calling another type predicate that does so first, which necessitates adding a few casts because tsc infers the type of the argument too narrowly). * fix(interfaces): Add missing check to hasBubble type predicate This appears to have inadvertently been omitted in PR #9004. * fix(interfaces): Fix misplaced typeof * fix: Fix typos in JSDocs * fix(tests): Make Mocks conform to corresponding interfaces Introduce a new MockFocusable, and add methods to MockIcon, MockBubbleIcon and MockComment, so that they fulfil the IFocusableNode, IIcon, IHasBubble and ICommentIcon interfaces respectively. * chore(tests): Add assertions verifying mocks conform to predicates Add (test) runtime assertions that: - isFocusableNode(MockFocusable) returns true - isIcon(MockIcon) returns true - hasBubble(MockBubbleIcon) returns true - isCommentIcon(MockCommentIcon) returns true (The latter is currently failing because Blockly is undefined when isCommentIcon calls the MockCommentIcon's getType method.) * fix(tests): Don't rely on Blockly being set in Mock methods For some reason the global Blockly binding is not visible at the time when isCommentIcon calls MockCommentIcon's getType method, and presumably this problem would apply to getBubbleSize too, so directly import the required items. * refactor(tests): Make MockCommentIcon a MockBubbleIcon This slightly simplifies it and makes it less likely to accidentally stop conforming to IHasBubble. * fix(interfaces): Fix incorrect check in isSelectable Fix an error which caused ISelectable instances to fail isSelectable() checks, one of the results of which is that Blockly.common.getSelected() would generally return null. Whoops! --- core/interfaces/i_autohideable.ts | 2 +- core/interfaces/i_comment_icon.ts | 14 ++++---- core/interfaces/i_copyable.ts | 2 +- core/interfaces/i_deletable.ts | 7 ++-- core/interfaces/i_draggable.ts | 13 ++++---- core/interfaces/i_focusable_node.ts | 16 ++++----- core/interfaces/i_focusable_tree.ts | 18 +++++----- core/interfaces/i_has_bubble.ts | 4 ++- core/interfaces/i_icon.ts | 28 ++++++++-------- core/interfaces/i_legacy_procedure_blocks.ts | 15 +++++---- core/interfaces/i_observable.ts | 6 +++- core/interfaces/i_paster.ts | 2 +- core/interfaces/i_procedure_block.ts | 7 ++-- core/interfaces/i_rendered_element.ts | 2 +- core/interfaces/i_selectable.ts | 12 +++---- core/interfaces/i_serializable.ts | 6 +++- tests/mocha/block_test.js | 19 ++++++----- tests/mocha/test_helpers/icon_mocks.js | 35 +++++++++++++++++++- 18 files changed, 128 insertions(+), 80 deletions(-) diff --git a/core/interfaces/i_autohideable.ts b/core/interfaces/i_autohideable.ts index 41e761f57..1193023d2 100644 --- a/core/interfaces/i_autohideable.ts +++ b/core/interfaces/i_autohideable.ts @@ -23,5 +23,5 @@ export interface IAutoHideable extends IComponent { /** Returns true if the given object is autohideable. */ export function isAutoHideable(obj: any): obj is IAutoHideable { - return obj.autoHide !== undefined; + return obj && typeof obj.autoHide === 'function'; } diff --git a/core/interfaces/i_comment_icon.ts b/core/interfaces/i_comment_icon.ts index 05f86f40f..1ab5bead4 100644 --- a/core/interfaces/i_comment_icon.ts +++ b/core/interfaces/i_comment_icon.ts @@ -31,17 +31,17 @@ export interface ICommentIcon extends IIcon, IHasBubble, ISerializable { } /** Checks whether the given object is an ICommentIcon. */ -export function isCommentIcon(obj: object): obj is ICommentIcon { +export function isCommentIcon(obj: any): obj is ICommentIcon { return ( isIcon(obj) && hasBubble(obj) && isSerializable(obj) && - (obj as any)['setText'] !== undefined && - (obj as any)['getText'] !== undefined && - (obj as any)['setBubbleSize'] !== undefined && - (obj as any)['getBubbleSize'] !== undefined && - (obj as any)['setBubbleLocation'] !== undefined && - (obj as any)['getBubbleLocation'] !== undefined && + typeof (obj as any).setText === 'function' && + typeof (obj as any).getText === 'function' && + typeof (obj as any).setBubbleSize === 'function' && + typeof (obj as any).getBubbleSize === 'function' && + typeof (obj as any).setBubbleLocation === 'function' && + typeof (obj as any).getBubbleLocation === 'function' && obj.getType() === IconType.COMMENT ); } diff --git a/core/interfaces/i_copyable.ts b/core/interfaces/i_copyable.ts index 6c354926a..8d1853967 100644 --- a/core/interfaces/i_copyable.ts +++ b/core/interfaces/i_copyable.ts @@ -35,5 +35,5 @@ export type ICopyData = ICopyable.ICopyData; /** @returns true if the given object is an ICopyable. */ export function isCopyable(obj: any): obj is ICopyable { - return obj.toCopyData !== undefined; + return obj && typeof obj.toCopyData === 'function'; } diff --git a/core/interfaces/i_deletable.ts b/core/interfaces/i_deletable.ts index 046770940..156e43ddc 100644 --- a/core/interfaces/i_deletable.ts +++ b/core/interfaces/i_deletable.ts @@ -27,8 +27,9 @@ export interface IDeletable { /** Returns whether the given object is an IDeletable. */ export function isDeletable(obj: any): obj is IDeletable { return ( - obj['isDeletable'] !== undefined && - obj['dispose'] !== undefined && - obj['setDeleteStyle'] !== undefined + obj && + typeof obj.isDeletable === 'function' && + typeof obj.dispose === 'function' && + typeof obj.setDeleteStyle === 'function' ); } diff --git a/core/interfaces/i_draggable.ts b/core/interfaces/i_draggable.ts index cb723e7b8..913038116 100644 --- a/core/interfaces/i_draggable.ts +++ b/core/interfaces/i_draggable.ts @@ -62,11 +62,12 @@ export interface IDragStrategy { /** Returns whether the given object is an IDraggable or not. */ export function isDraggable(obj: any): obj is IDraggable { return ( - obj.getRelativeToSurfaceXY !== undefined && - obj.isMovable !== undefined && - obj.startDrag !== undefined && - obj.drag !== undefined && - obj.endDrag !== undefined && - obj.revertDrag !== undefined + obj && + typeof obj.getRelativeToSurfaceXY === 'function' && + typeof obj.isMovable === 'function' && + typeof obj.startDrag === 'function' && + typeof obj.drag === 'function' && + typeof obj.endDrag === 'function' && + typeof obj.revertDrag === 'function' ); } diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 00557168a..24833328d 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -102,16 +102,16 @@ export interface IFocusableNode { * Determines whether the provided object fulfills the contract of * IFocusableNode. * - * @param object The object to test. + * @param obj The object to test. * @returns Whether the provided object can be used as an IFocusableNode. */ -export function isFocusableNode(object: any | null): object is IFocusableNode { +export function isFocusableNode(obj: any): obj is IFocusableNode { return ( - object && - 'getFocusableElement' in object && - 'getFocusableTree' in object && - 'onNodeFocus' in object && - 'onNodeBlur' in object && - 'canBeFocused' in object + obj && + typeof obj.getFocusableElement === 'function' && + typeof obj.getFocusableTree === 'function' && + typeof obj.onNodeFocus === 'function' && + typeof obj.onNodeBlur === 'function' && + typeof obj.canBeFocused === 'function' ); } diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index f4f25f7f5..c33189fcd 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -128,17 +128,17 @@ export interface IFocusableTree { * Determines whether the provided object fulfills the contract of * IFocusableTree. * - * @param object The object to test. + * @param obj The object to test. * @returns Whether the provided object can be used as an IFocusableTree. */ -export function isFocusableTree(object: any | null): object is IFocusableTree { +export function isFocusableTree(obj: any): obj is IFocusableTree { return ( - object && - 'getRootFocusableNode' in object && - 'getRestoredFocusableNode' in object && - 'getNestedTrees' in object && - 'lookUpFocusableNode' in object && - 'onTreeFocus' in object && - 'onTreeBlur' in object + obj && + typeof obj.getRootFocusableNode === 'function' && + typeof obj.getRestoredFocusableNode === 'function' && + typeof obj.getNestedTrees === 'function' && + typeof obj.lookUpFocusableNode === 'function' && + typeof obj.onTreeFocus === 'function' && + typeof obj.onTreeBlur === 'function' ); } diff --git a/core/interfaces/i_has_bubble.ts b/core/interfaces/i_has_bubble.ts index 85c6f0990..0c2e257a4 100644 --- a/core/interfaces/i_has_bubble.ts +++ b/core/interfaces/i_has_bubble.ts @@ -30,6 +30,8 @@ export interface IHasBubble { /** Type guard that checks whether the given object is a IHasBubble. */ export function hasBubble(obj: any): obj is IHasBubble { return ( - obj.bubbleIsVisible !== undefined && obj.setBubbleVisible !== undefined + typeof obj.bubbleIsVisible === 'function' && + typeof obj.setBubbleVisible === 'function' && + typeof obj.getBubble === 'function' ); } diff --git a/core/interfaces/i_icon.ts b/core/interfaces/i_icon.ts index 74489dc5e..06f416424 100644 --- a/core/interfaces/i_icon.ts +++ b/core/interfaces/i_icon.ts @@ -98,19 +98,19 @@ export interface IIcon extends IFocusableNode { /** Type guard that checks whether the given object is an IIcon. */ export function isIcon(obj: any): obj is IIcon { return ( - obj.getType !== undefined && - obj.initView !== undefined && - obj.dispose !== undefined && - obj.getWeight !== undefined && - obj.getSize !== undefined && - obj.applyColour !== undefined && - obj.hideForInsertionMarker !== undefined && - obj.updateEditable !== undefined && - obj.updateCollapsed !== undefined && - obj.isShownWhenCollapsed !== undefined && - obj.setOffsetInBlock !== undefined && - obj.onLocationChange !== undefined && - obj.onClick !== undefined && - isFocusableNode(obj) + isFocusableNode(obj) && + typeof (obj as IIcon).getType === 'function' && + typeof (obj as IIcon).initView === 'function' && + typeof (obj as IIcon).dispose === 'function' && + typeof (obj as IIcon).getWeight === 'function' && + typeof (obj as IIcon).getSize === 'function' && + typeof (obj as IIcon).applyColour === 'function' && + typeof (obj as IIcon).hideForInsertionMarker === 'function' && + typeof (obj as IIcon).updateEditable === 'function' && + typeof (obj as IIcon).updateCollapsed === 'function' && + typeof (obj as IIcon).isShownWhenCollapsed === 'function' && + typeof (obj as IIcon).setOffsetInBlock === 'function' && + typeof (obj as IIcon).onLocationChange === 'function' && + typeof (obj as IIcon).onClick === 'function' ); } diff --git a/core/interfaces/i_legacy_procedure_blocks.ts b/core/interfaces/i_legacy_procedure_blocks.ts index d74eaec22..c723a5ed7 100644 --- a/core/interfaces/i_legacy_procedure_blocks.ts +++ b/core/interfaces/i_legacy_procedure_blocks.ts @@ -28,9 +28,9 @@ export interface LegacyProcedureDefBlock { /** @internal */ export function isLegacyProcedureDefBlock( - block: object, -): block is LegacyProcedureDefBlock { - return (block as any).getProcedureDef !== undefined; + obj: any, +): obj is LegacyProcedureDefBlock { + return obj && typeof obj.getProcedureDef === 'function'; } /** @internal */ @@ -41,10 +41,11 @@ export interface LegacyProcedureCallBlock { /** @internal */ export function isLegacyProcedureCallBlock( - block: object, -): block is LegacyProcedureCallBlock { + obj: any, +): obj is LegacyProcedureCallBlock { return ( - (block as any).getProcedureCall !== undefined && - (block as any).renameProcedure !== undefined + obj && + typeof obj.getProcedureCall === 'function' && + typeof obj.renameProcedure === 'function' ); } diff --git a/core/interfaces/i_observable.ts b/core/interfaces/i_observable.ts index 96a2a0bc4..8db0c2378 100644 --- a/core/interfaces/i_observable.ts +++ b/core/interfaces/i_observable.ts @@ -20,5 +20,9 @@ export interface IObservable { * @internal */ export function isObservable(obj: any): obj is IObservable { - return obj.startPublishing !== undefined && obj.stopPublishing !== undefined; + return ( + obj && + typeof obj.startPublishing === 'function' && + typeof obj.stopPublishing === 'function' + ); } diff --git a/core/interfaces/i_paster.ts b/core/interfaces/i_paster.ts index 321ff118f..128913a26 100644 --- a/core/interfaces/i_paster.ts +++ b/core/interfaces/i_paster.ts @@ -21,5 +21,5 @@ export interface IPaster> { export function isPaster( obj: any, ): obj is IPaster> { - return obj.paste !== undefined; + return obj && typeof obj.paste === 'function'; } diff --git a/core/interfaces/i_procedure_block.ts b/core/interfaces/i_procedure_block.ts index f85380527..3a6dc4847 100644 --- a/core/interfaces/i_procedure_block.ts +++ b/core/interfaces/i_procedure_block.ts @@ -20,9 +20,10 @@ export interface IProcedureBlock { export function isProcedureBlock( block: Block | IProcedureBlock, ): block is IProcedureBlock { + block = block as IProcedureBlock; return ( - (block as IProcedureBlock).getProcedureModel !== undefined && - (block as IProcedureBlock).doProcedureUpdate !== undefined && - (block as IProcedureBlock).isProcedureDef !== undefined + typeof block.getProcedureModel === 'function' && + typeof block.doProcedureUpdate === 'function' && + typeof block.isProcedureDef === 'function' ); } diff --git a/core/interfaces/i_rendered_element.ts b/core/interfaces/i_rendered_element.ts index fe9460c7f..2f82487e9 100644 --- a/core/interfaces/i_rendered_element.ts +++ b/core/interfaces/i_rendered_element.ts @@ -15,5 +15,5 @@ export interface IRenderedElement { * @returns True if the given object is an IRenderedElement. */ export function isRenderedElement(obj: any): obj is IRenderedElement { - return obj['getSvgRoot'] !== undefined; + return obj && typeof obj.getSvgRoot === 'function'; } diff --git a/core/interfaces/i_selectable.ts b/core/interfaces/i_selectable.ts index 639972e45..5374f50cd 100644 --- a/core/interfaces/i_selectable.ts +++ b/core/interfaces/i_selectable.ts @@ -30,12 +30,12 @@ export interface ISelectable extends IFocusableNode { } /** Checks whether the given object is an ISelectable. */ -export function isSelectable(obj: object): obj is ISelectable { +export function isSelectable(obj: any): obj is ISelectable { return ( - typeof (obj as any).id === 'string' && - (obj as any).workspace !== undefined && - (obj as any).select !== undefined && - (obj as any).unselect !== undefined && - isFocusableNode(obj) + isFocusableNode(obj) && + typeof (obj as ISelectable).id === 'string' && + typeof (obj as ISelectable).workspace === 'object' && + typeof (obj as ISelectable).select === 'function' && + typeof (obj as ISelectable).unselect === 'function' ); } diff --git a/core/interfaces/i_serializable.ts b/core/interfaces/i_serializable.ts index 380a27709..99e597da3 100644 --- a/core/interfaces/i_serializable.ts +++ b/core/interfaces/i_serializable.ts @@ -24,5 +24,9 @@ export interface ISerializable { /** Type guard that checks whether the given object is a ISerializable. */ export function isSerializable(obj: any): obj is ISerializable { - return obj.saveState !== undefined && obj.loadState !== undefined; + return ( + obj && + typeof obj.saveState === 'function' && + typeof obj.loadState === 'function' + ); } diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index eda2d82a5..62c61ce00 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -7,7 +7,10 @@ import {ConnectionType} from '../../build/src/core/connection_type.js'; import {EventType} from '../../build/src/core/events/type.js'; import * as eventUtils from '../../build/src/core/events/utils.js'; +import {IconType} from '../../build/src/core/icons/icon_types.js'; import {EndRowInput} from '../../build/src/core/inputs/end_row_input.js'; +import {isCommentIcon} from '../../build/src/core/interfaces/i_comment_icon.js'; +import {Size} from '../../build/src/core/utils/size.js'; import {assert} from '../../node_modules/chai/chai.js'; import {createRenderedBlock} from './test_helpers/block_definitions.js'; import { @@ -1426,9 +1429,9 @@ suite('Blocks', function () { }); suite('Constructing registered comment classes', function () { - class MockComment extends MockIcon { + class MockComment extends MockBubbleIcon { getType() { - return Blockly.icons.IconType.COMMENT; + return IconType.COMMENT; } setText() {} @@ -1440,19 +1443,13 @@ suite('Blocks', function () { setBubbleSize() {} getBubbleSize() { - return Blockly.utils.Size(0, 0); + return Size(0, 0); } setBubbleLocation() {} getBubbleLocation() {} - bubbleIsVisible() { - return true; - } - - setBubbleVisible() {} - saveState() { return {}; } @@ -1460,6 +1457,10 @@ suite('Blocks', function () { loadState() {} } + if (!isCommentIcon(new MockComment())) { + throw new TypeError('MockComment not an ICommentIcon'); + } + setup(function () { this.workspace = Blockly.inject('blocklyDiv', {}); diff --git a/tests/mocha/test_helpers/icon_mocks.js b/tests/mocha/test_helpers/icon_mocks.js index 5d117c712..0e549b976 100644 --- a/tests/mocha/test_helpers/icon_mocks.js +++ b/tests/mocha/test_helpers/icon_mocks.js @@ -4,7 +4,24 @@ * SPDX-License-Identifier: Apache-2.0 */ -export class MockIcon { +import {isFocusableNode} from '../../../build/src/core/interfaces/i_focusable_node.js'; +import {hasBubble} from '../../../build/src/core/interfaces/i_has_bubble.js'; +import {isIcon} from '../../../build/src/core/interfaces/i_icon.js'; +import {isSerializable} from '../../../build/src/core/interfaces/i_serializable.js'; + +export class MockFocusable { + getFocusableElement() {} + getFocusableTree() {} + onNodeFocus() {} + onNodeBlur() {} + canBeFocused() {} +} + +if (!isFocusableNode(new MockFocusable())) { + throw new TypeError('MockFocusable not an IFocuableNode'); +} + +export class MockIcon extends MockFocusable { getType() { return new Blockly.icons.IconType('mock icon'); } @@ -52,6 +69,10 @@ export class MockIcon { } } +if (!isIcon(new MockIcon())) { + throw new TypeError('MockIcon not an IIcon'); +} + export class MockSerializableIcon extends MockIcon { constructor() { super(); @@ -75,6 +96,10 @@ export class MockSerializableIcon extends MockIcon { } } +if (!isSerializable(new MockSerializableIcon())) { + throw new TypeError('MockSerializableIcon not an ISerializable'); +} + export class MockBubbleIcon extends MockIcon { constructor() { super(); @@ -94,4 +119,12 @@ export class MockBubbleIcon extends MockIcon { setBubbleVisible(visible) { this.visible = visible; } + + getBubble() { + return null; + } +} + +if (!hasBubble(new MockBubbleIcon())) { + throw new TypeError('MockBubbleIcon not an IHasBubble'); }