From faa95d022d369584402c08291deb082900f18406 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Thu, 23 Mar 2023 20:30:48 +0000 Subject: [PATCH] fix(fields): Make SKIP_SETUP a Symbol; remove Sentinel class (#6919) We introduced the SKIP_SETUP sentinel value when converting Field and its subclasses to ES6 class syntax, because super must be called before any other code in a subclass constructor, breaking the previous mechanism where subclasses would set some properties before calling their superclass constructor. SKIP_SETUP was a singleton value of class Sentinel. Recently, in PR #6639 @btw17 introduced the isSentinel type predicate to improve the typing of Field. Unfortunately, there were some aspects of this change that were not very elegant: - isSentinel was declared as a static method on Field (rather than on Sentinel itself). - It only checks against the specific value SKIP_SETUP, rather than checking if the argument is instanceof Sentinel (though perhaps this is for efficiency?) Additionally - as a result of the migration from ES6 to TS, and predating PR #6639 - The signature for the Field constructor's first argument was typed T|Sentinel, with subclass constructors generally being |Sentinel. This creates a small problem when attempting to port Fields from core to plugins, because Sentinel is not reexported by core/utils.ts (and therefore not from core/blockly.ts either). The latter problem could be solved simply by reexporting Sentinel, or by having plugin constructors not accept SKIP_SETUP (though this potentially makes them more difficult to subclass), but neither is particularly desirable. Instead, this PR proposes that we: - Make Field.SKIP_SETUP a (unique) Symbol. - Change the value argument to the Field constructor to accept T|typeof Field.SKIP_SETUP - where typeof Field.SKIP_SETUP is (like a literal type) a type that accepts just the single value SKIP_SETUP. - Remove the Sentinel class and core/utils/sentinel.ts. Not treating this as a breaking change: - Removes Field.isSentinel - though this addition has not yet been published, so it can only break our own as-yet-unreleased code in samples. - Changes the type of Field.SKIP_SETUP and the first argument of the Field constructor from Sentinel to typeof SKIP_SETUP (a unique Symbol) - but given that Sentinel has never been exported this should not break any actual external code. --- core/field.ts | 10 +++------- core/field_angle.ts | 7 +++---- core/field_checkbox.ts | 7 +++---- core/field_colour.ts | 5 ++--- core/field_dropdown.ts | 16 +++------------- core/field_image.ts | 11 ++++------- core/field_input.ts | 7 +++---- core/field_label.ts | 6 +++--- core/field_multilineinput.ts | 6 +++--- core/field_number.ts | 5 ++--- core/field_textinput.ts | 6 +++--- core/field_variable.ts | 11 ++++------- core/utils/sentinel.ts | 25 ------------------------- 13 files changed, 36 insertions(+), 86 deletions(-) delete mode 100644 core/utils/sentinel.ts diff --git a/core/field.ts b/core/field.ts index ff5663599..eda991fe4 100644 --- a/core/field.ts +++ b/core/field.ts @@ -35,7 +35,6 @@ import type {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; import {Rect} from './utils/rect.js'; -import {Sentinel} from './utils/sentinel.js'; import {Size} from './utils/size.js'; import * as style from './utils/style.js'; import {Svg} from './utils/svg.js'; @@ -89,10 +88,7 @@ export abstract class Field implements IASTNodeLocationSvg, * field's value or run configure_, and should allow a subclass to do that * instead. */ - static readonly SKIP_SETUP = new Sentinel(); - static isSentinel(value: T|Sentinel): value is Sentinel { - return value === Field.SKIP_SETUP; - } + static readonly SKIP_SETUP = Symbol('SKIP_SETUP'); /** * Name of field. Unique within each block. @@ -211,7 +207,7 @@ export abstract class Field implements IASTNodeLocationSvg, * this parameter supports. */ constructor( - value: T|Sentinel, validator?: FieldValidator|null, + value: T|typeof Field.SKIP_SETUP, validator?: FieldValidator|null, config?: FieldConfig) { /** * A generic value possessed by the field. @@ -224,7 +220,7 @@ export abstract class Field implements IASTNodeLocationSvg, /** The size of the area rendered by the field. */ this.size_ = new Size(0, 0); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_angle.ts b/core/field_angle.ts index c2f9318e7..e5fbbf35a 100644 --- a/core/field_angle.ts +++ b/core/field_angle.ts @@ -21,7 +21,6 @@ import * as fieldRegistry from './field_registry.js'; import {FieldInput, FieldInputConfig, FieldInputValidator} from './field_input.js'; import * as dom from './utils/dom.js'; import * as math from './utils/math.js'; -import type {Sentinel} from './utils/sentinel.js'; import {Svg} from './utils/svg.js'; import * as userAgent from './utils/useragent.js'; import * as WidgetDiv from './widgetdiv.js'; @@ -115,11 +114,11 @@ export class FieldAngle extends FieldInput { * for a list of properties this parameter supports. */ constructor( - value?: string|number|Sentinel, validator?: FieldAngleValidator, - config?: FieldAngleConfig) { + value?: string|number|typeof Field.SKIP_SETUP, + validator?: FieldAngleValidator, config?: FieldAngleConfig) { super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index 31cc50134..3f26d6700 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -18,7 +18,6 @@ import './events/events_block_change.js'; import * as dom from './utils/dom.js'; import {Field, FieldConfig, FieldValidator} from './field.js'; import * as fieldRegistry from './field_registry.js'; -import type {Sentinel} from './utils/sentinel.js'; type BoolString = 'TRUE'|'FALSE'; type CheckboxBool = BoolString|boolean; @@ -63,8 +62,8 @@ export class FieldCheckbox extends Field { * for a list of properties this parameter supports. */ constructor( - value?: CheckboxBool|Sentinel, validator?: FieldCheckboxValidator, - config?: FieldCheckboxConfig) { + value?: CheckboxBool|typeof Field.SKIP_SETUP, + validator?: FieldCheckboxValidator, config?: FieldCheckboxConfig) { super(Field.SKIP_SETUP); /** @@ -73,7 +72,7 @@ export class FieldCheckbox extends Field { */ this.checkChar_ = FieldCheckbox.CHECK_CHAR; - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_colour.ts b/core/field_colour.ts index 63fd1a752..87c34b99d 100644 --- a/core/field_colour.ts +++ b/core/field_colour.ts @@ -25,7 +25,6 @@ import * as fieldRegistry from './field_registry.js'; import * as aria from './utils/aria.js'; import * as colour from './utils/colour.js'; import * as idGenerator from './utils/idgenerator.js'; -import type {Sentinel} from './utils/sentinel.js'; import {Size} from './utils/size.js'; /** @@ -133,11 +132,11 @@ export class FieldColour extends Field { * for a list of properties this parameter supports. */ constructor( - value?: string|Sentinel, validator?: FieldColourValidator, + value?: string|typeof Field.SKIP_SETUP, validator?: FieldColourValidator, config?: FieldColourConfig) { super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 0650be4c3..9b1fdf615 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -25,7 +25,6 @@ import * as aria from './utils/aria.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; import * as utilsString from './utils/string.js'; import {Svg} from './utils/svg.js'; @@ -113,16 +112,16 @@ export class FieldDropdown extends Field { validator?: FieldDropdownValidator, config?: FieldDropdownConfig, ); - constructor(menuGenerator: Sentinel); + constructor(menuGenerator: typeof Field.SKIP_SETUP); constructor( - menuGenerator: MenuGenerator|Sentinel, + menuGenerator: MenuGenerator|typeof Field.SKIP_SETUP, validator?: FieldDropdownValidator, config?: FieldDropdownConfig, ) { super(Field.SKIP_SETUP); // If we pass SKIP_SETUP, don't do *anything* with the menu generator. - if (!isMenuGenerator(menuGenerator)) return; + if (menuGenerator === Field.SKIP_SETUP) return; if (Array.isArray(menuGenerator)) { validateOptions(menuGenerator); @@ -690,15 +689,6 @@ const IMAGE_Y_OFFSET = 5; /** The total vertical padding above and below an image. */ const IMAGE_Y_PADDING: number = IMAGE_Y_OFFSET * 2; -/** - * NOTE: Because Sentinel is an empty class, proving a value is Sentinel does - * not resolve in TS that it isn't a MenuGenerator. - */ -function isMenuGenerator(menuGenerator: MenuGenerator| - Sentinel): menuGenerator is MenuGenerator { - return menuGenerator !== Field.SKIP_SETUP; -} - /** * Factor out common words in statically defined options. * Create prefix and/or suffix labels. diff --git a/core/field_image.ts b/core/field_image.ts index 60ddac433..ecd702f3f 100644 --- a/core/field_image.ts +++ b/core/field_image.ts @@ -16,7 +16,6 @@ import {Field, FieldConfig} from './field.js'; import * as fieldRegistry from './field_registry.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; import {Size} from './utils/size.js'; import {Svg} from './utils/svg.js'; @@ -74,9 +73,9 @@ export class FieldImage extends Field { * for a list of properties this parameter supports. */ constructor( - src: string|Sentinel, width: string|number, height: string|number, - alt?: string, onClick?: (p1: FieldImage) => void, flipRtl?: boolean, - config?: FieldImageConfig) { + src: string|typeof Field.SKIP_SETUP, width: string|number, + height: string|number, alt?: string, onClick?: (p1: FieldImage) => void, + flipRtl?: boolean, config?: FieldImageConfig) { super(Field.SKIP_SETUP); const imageHeight = Number(parsing.replaceMessageReferences(height)); @@ -104,9 +103,7 @@ export class FieldImage extends Field { this.clickHandler_ = onClick; } - if (src === Field.SKIP_SETUP) { - return; - } + if (src === Field.SKIP_SETUP) return; if (config) { this.configure_(config); diff --git a/core/field_input.ts b/core/field_input.ts index 756103d16..0257a345a 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -25,7 +25,6 @@ import {Field, FieldConfig, FieldValidator, UnattachedFieldError} from './field. import {Msg} from './msg.js'; import * as aria from './utils/aria.js'; import {Coordinate} from './utils/coordinate.js'; -import type {Sentinel} from './utils/sentinel.js'; import * as userAgent from './utils/useragent.js'; import * as WidgetDiv from './widgetdiv.js'; import type {WorkspaceSvg} from './workspace_svg.js'; @@ -103,11 +102,11 @@ export abstract class FieldInput extends Field { * for a list of properties this parameter supports. */ constructor( - value?: string|Sentinel, validator?: FieldInputValidator|null, - config?: FieldInputConfig) { + value?: string|typeof Field.SKIP_SETUP, + validator?: FieldInputValidator|null, config?: FieldInputConfig) { super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_label.ts b/core/field_label.ts index 4cb293ef6..024646495 100644 --- a/core/field_label.ts +++ b/core/field_label.ts @@ -17,7 +17,6 @@ import * as dom from './utils/dom.js'; import {Field, FieldConfig} from './field.js'; import * as fieldRegistry from './field_registry.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; /** * Class for a non-editable, non-serializable text field. @@ -45,10 +44,11 @@ export class FieldLabel extends Field { * for a list of properties this parameter supports. */ constructor( - value?: string|Sentinel, textClass?: string, config?: FieldLabelConfig) { + value?: string|typeof Field.SKIP_SETUP, textClass?: string, + config?: FieldLabelConfig) { super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } else { diff --git a/core/field_multilineinput.ts b/core/field_multilineinput.ts index 40f82de23..1472c8996 100644 --- a/core/field_multilineinput.ts +++ b/core/field_multilineinput.ts @@ -19,7 +19,6 @@ import {FieldTextInput, FieldTextInputConfig, FieldTextInputValidator} from './f import * as aria from './utils/aria.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; import {Svg} from './utils/svg.js'; import * as userAgent from './utils/useragent.js'; import * as WidgetDiv from './widgetdiv.js'; @@ -59,11 +58,12 @@ export class FieldMultilineInput extends FieldTextInput { * for a list of properties this parameter supports. */ constructor( - value?: string|Sentinel, validator?: FieldMultilineInputValidator, + value?: string|typeof Field.SKIP_SETUP, + validator?: FieldMultilineInputValidator, config?: FieldMultilineInputConfig) { super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } diff --git a/core/field_number.ts b/core/field_number.ts index 262373050..72f383d01 100644 --- a/core/field_number.ts +++ b/core/field_number.ts @@ -16,7 +16,6 @@ import {Field} from './field.js'; import * as fieldRegistry from './field_registry.js'; import {FieldInput, FieldInputConfig, FieldInputValidator} from './field_input.js'; import * as aria from './utils/aria.js'; -import type {Sentinel} from './utils/sentinel.js'; /** * Class for an editable number field. @@ -60,13 +59,13 @@ export class FieldNumber extends FieldInput { * for a list of properties this parameter supports. */ constructor( - value?: string|number|Sentinel, min?: string|number|null, + value?: string|number|typeof Field.SKIP_SETUP, min?: string|number|null, max?: string|number|null, precision?: string|number|null, validator?: FieldNumberValidator|null, config?: FieldNumberConfig) { // Pass SENTINEL so that we can define properties before value validation. super(Field.SKIP_SETUP); - if (Field.isSentinel(value)) return; + if (value === Field.SKIP_SETUP) return; if (config) { this.configure_(config); } else { diff --git a/core/field_textinput.ts b/core/field_textinput.ts index f519f98bb..600e14cb1 100644 --- a/core/field_textinput.ts +++ b/core/field_textinput.ts @@ -15,10 +15,10 @@ goog.declareModuleId('Blockly.FieldTextInput'); // Unused import preserved for side-effects. Remove if unneeded. import './events/events_block_change.js'; +import {Field} from './field.js'; import {FieldInput, FieldInputConfig, FieldInputValidator} from './field_input.js'; import * as fieldRegistry from './field_registry.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; /** * Class for an editable text field. @@ -39,8 +39,8 @@ export class FieldTextInput extends FieldInput { * for a list of properties this parameter supports. */ constructor( - value?: string|Sentinel, validator?: FieldTextInputValidator|null, - config?: FieldTextInputConfig) { + value?: string|typeof Field.SKIP_SETUP, + validator?: FieldTextInputValidator|null, config?: FieldTextInputConfig) { super(value, validator, config); } diff --git a/core/field_variable.ts b/core/field_variable.ts index 61159463e..5bde39de1 100644 --- a/core/field_variable.ts +++ b/core/field_variable.ts @@ -24,7 +24,6 @@ import type {Menu} from './menu.js'; import type {MenuItem} from './menuitem.js'; import {Msg} from './msg.js'; import * as parsing from './utils/parsing.js'; -import type {Sentinel} from './utils/sentinel.js'; import {Size} from './utils/size.js'; import {VariableModel} from './variable_model.js'; import * as Variables from './variables.js'; @@ -76,9 +75,9 @@ export class FieldVariable extends FieldDropdown { * for a list of properties this parameter supports. */ constructor( - varName: string|null|Sentinel, validator?: FieldVariableValidator, - variableTypes?: string[], defaultType?: string, - config?: FieldVariableConfig) { + varName: string|null|typeof Field.SKIP_SETUP, + validator?: FieldVariableValidator, variableTypes?: string[], + defaultType?: string, config?: FieldVariableConfig) { super(Field.SKIP_SETUP); /** @@ -97,9 +96,7 @@ export class FieldVariable extends FieldDropdown { /** The size of the area rendered by the field. */ this.size_ = new Size(0, 0); - if (varName === Field.SKIP_SETUP) { - return; - } + if (varName === Field.SKIP_SETUP) return; if (config) { this.configure_(config); diff --git a/core/utils/sentinel.ts b/core/utils/sentinel.ts deleted file mode 100644 index 6f71ee9d5..000000000 --- a/core/utils/sentinel.ts +++ /dev/null @@ -1,25 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * A type used to create flag values. - * - * @class - */ -import * as goog from '../../closure/goog/goog.js'; -goog.declareModuleId('Blockly.utils.Sentinel'); - - -/** - * A type used to create flag values. - */ -export class Sentinel { - /** - * Provide a unique key so that type guarding properly excludes values like - * string. - */ - UNIQUE_KEY?: never; -}