From 28ac0c44732d45114761392528fe706ff76c2f4a Mon Sep 17 00:00:00 2001 From: Maribeth Bottorff Date: Fri, 10 May 2024 14:14:50 -0700 Subject: [PATCH] fix: improve types in FieldRegistry (#8062) * fix: improve types in FieldRegistry * chore: tsdoc --- core/field.ts | 24 ++++++++++++-- core/field_angle.ts | 2 +- core/field_checkbox.ts | 4 ++- core/field_colour.ts | 2 +- core/field_dropdown.ts | 4 ++- core/field_image.ts | 2 +- core/field_label.ts | 2 +- core/field_number.ts | 2 +- core/field_registry.ts | 51 ++++++++++++++++++++++++------ core/field_textinput.ts | 4 ++- tests/mocha/field_registry_test.js | 22 ++++++++++--- 11 files changed, 95 insertions(+), 24 deletions(-) diff --git a/core/field.ts b/core/field.ts index fb6c078e9..58f120cb8 100644 --- a/core/field.ts +++ b/core/field.ts @@ -1433,6 +1433,22 @@ export abstract class Field workspace.getMarker(MarkerManager.LOCAL_MARKER)!.draw(); } } + + /** + * Subclasses should reimplement this method to construct their Field + * subclass from a JSON arg object. + * + * It is an error to attempt to register a field subclass in the + * FieldRegistry if that subclass has not overridden this method. + * + * @param _options JSON configuration object with properties needed + * to configure a specific field. + */ + static fromJson(_options: FieldConfig): Field { + throw new Error( + `Attempted to instantiate a field from the registry that hasn't defined a 'fromJson' method.`, + ); + } } /** @@ -1443,12 +1459,14 @@ export interface FieldConfig { } /** - * For use by Field and descendants of Field. Constructors can change + * Represents an object that has all the prototype properties of the `Field` + * class. This is necessary because constructors can change * in descendants, though they should contain all of Field's prototype methods. * - * @internal + * This type should only be used in places where we directly access the prototype + * of a Field class or subclass. */ -export type FieldProto = Pick; +type FieldProto = Pick; /** * Represents an error where the field is trying to access its block or diff --git a/core/field_angle.ts b/core/field_angle.ts index 9e94ea4d8..7eef3099f 100644 --- a/core/field_angle.ts +++ b/core/field_angle.ts @@ -516,7 +516,7 @@ export class FieldAngle extends FieldInput { * @nocollapse * @internal */ - static fromJson(options: FieldAngleFromJsonConfig): FieldAngle { + static override fromJson(options: FieldAngleFromJsonConfig): FieldAngle { // `this` might be a subclass of FieldAngle if that class doesn't override // the static fromJson method. return new this(options.angle, undefined, options); diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index 7bda3a58f..83f460bb9 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -226,7 +226,9 @@ export class FieldCheckbox extends Field { * @nocollapse * @internal */ - static fromJson(options: FieldCheckboxFromJsonConfig): FieldCheckbox { + static override fromJson( + options: FieldCheckboxFromJsonConfig, + ): FieldCheckbox { // `this` might be a subclass of FieldCheckbox if that class doesn't // 'override' the static fromJson method. return new this(options.checked, undefined, options); diff --git a/core/field_colour.ts b/core/field_colour.ts index 6b78a2e50..46bf3f0e2 100644 --- a/core/field_colour.ts +++ b/core/field_colour.ts @@ -641,7 +641,7 @@ export class FieldColour extends Field { * @nocollapse * @internal */ - static fromJson(options: FieldColourFromJsonConfig): FieldColour { + static override fromJson(options: FieldColourFromJsonConfig): FieldColour { // `this` might be a subclass of FieldColour if that class doesn't override // the static fromJson method. return new this(options.colour, undefined, options); diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index d92d02ec2..58a4b0732 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -647,7 +647,9 @@ export class FieldDropdown extends Field { * @nocollapse * @internal */ - static fromJson(options: FieldDropdownFromJsonConfig): FieldDropdown { + static override fromJson( + options: FieldDropdownFromJsonConfig, + ): FieldDropdown { if (!options.options) { throw new Error( 'options are required for the dropdown field. The ' + diff --git a/core/field_image.ts b/core/field_image.ts index 6d31a9772..09461a790 100644 --- a/core/field_image.ts +++ b/core/field_image.ts @@ -250,7 +250,7 @@ export class FieldImage extends Field { * @nocollapse * @internal */ - static fromJson(options: FieldImageFromJsonConfig): FieldImage { + static override fromJson(options: FieldImageFromJsonConfig): FieldImage { if (!options.src || !options.width || !options.height) { throw new Error( 'src, width, and height values for an image field are' + diff --git a/core/field_label.ts b/core/field_label.ts index 7409ca594..2b77b0d25 100644 --- a/core/field_label.ts +++ b/core/field_label.ts @@ -117,7 +117,7 @@ export class FieldLabel extends Field { * @nocollapse * @internal */ - static fromJson(options: FieldLabelFromJsonConfig): FieldLabel { + static override fromJson(options: FieldLabelFromJsonConfig): FieldLabel { const text = parsing.replaceMessageReferences(options.text); // `this` might be a subclass of FieldLabel if that class doesn't override // the static fromJson method. diff --git a/core/field_number.ts b/core/field_number.ts index ee04eab49..e8e51d060 100644 --- a/core/field_number.ts +++ b/core/field_number.ts @@ -315,7 +315,7 @@ export class FieldNumber extends FieldInput { * @nocollapse * @internal */ - static fromJson(options: FieldNumberFromJsonConfig): FieldNumber { + static override fromJson(options: FieldNumberFromJsonConfig): FieldNumber { // `this` might be a subclass of FieldNumber if that class doesn't override // the static fromJson method. return new this( diff --git a/core/field_registry.ts b/core/field_registry.ts index 0bd9e5626..06bb9acd0 100644 --- a/core/field_registry.ts +++ b/core/field_registry.ts @@ -6,12 +6,46 @@ // Former goog.module ID: Blockly.fieldRegistry -import type {Field, FieldProto} from './field.js'; +import type {Field, FieldConfig} from './field.js'; import * as registry from './registry.js'; -interface RegistryOptions { +/** + * When constructing a field from JSON using the registry, the + * `fromJson` method in this file is called with an options parameter + * object consisting of the "type" which is the name of the field, and + * other options that are part of the field's config object. + * + * These options are then passed to the field's static `fromJson` + * method. That method accepts an options parameter with a type that usually + * extends from FieldConfig, and may or may not have a "type" attribute (in + * fact, it shouldn't, because we'd overwrite it as described above!) + * + * Unfortunately the registry has no way of knowing the actual Field subclass + * that will be returned from passing in the name of the field. Therefore it + * also has no way of knowing that the options object not only implements + * `FieldConfig`, but it also should satisfy the Config that belongs to that + * specific class's `fromJson` method. + * + * Because of this uncertainty, we just give up on type checking the properties + * passed to the `fromJson` method, and allow arbitrary string keys with + * unknown types. + */ +type RegistryOptions = FieldConfig & { + // The name of the field, e.g. field_dropdown type: string; [key: string]: unknown; +}; + +/** + * Represents the static methods that must be defined on any + * field that is registered, i.e. the constructor and fromJson methods. + * + * Because we don't know which Field subclass will be registered, we + * are unable to typecheck the parameters of the constructor. + */ +export interface RegistrableField { + new (...args: any[]): Field; + fromJson(options: FieldConfig): Field; } /** @@ -25,7 +59,7 @@ interface RegistryOptions { * @throws {Error} if the type name is empty, the field is already registered, * or the fieldClass is not an object containing a fromJson function. */ -export function register(type: string, fieldClass: FieldProto) { +export function register(type: string, fieldClass: RegistrableField) { registry.register(registry.Type.FIELD, type, fieldClass); } @@ -59,7 +93,10 @@ export function fromJson(options: RegistryOptions): Field | null { * @param options */ function fromJsonInternal(options: RegistryOptions): Field | null { - const fieldObject = registry.getObject(registry.Type.FIELD, options.type); + const fieldObject = registry.getObject( + registry.Type.FIELD, + options.type, + ) as unknown as RegistrableField; if (!fieldObject) { console.warn( 'Blockly could not create a field of type ' + @@ -69,12 +106,8 @@ function fromJsonInternal(options: RegistryOptions): Field | null { ' #1584), or the registration is not being reached.', ); return null; - } else if (typeof (fieldObject as any).fromJson !== 'function') { - throw new TypeError('returned Field was not a IRegistrableField'); - } else { - type fromJson = (options: {}) => Field; - return (fieldObject as unknown as {fromJson: fromJson}).fromJson(options); } + return fieldObject.fromJson(options); } export const TEST_ONLY = { diff --git a/core/field_textinput.ts b/core/field_textinput.ts index 39e82b5a4..39bdca970 100644 --- a/core/field_textinput.ts +++ b/core/field_textinput.ts @@ -73,7 +73,9 @@ export class FieldTextInput extends FieldInput { * @nocollapse * @internal */ - static fromJson(options: FieldTextInputFromJsonConfig): FieldTextInput { + static override fromJson( + options: FieldTextInputFromJsonConfig, + ): FieldTextInput { const text = parsing.replaceMessageReferences(options.text); // `this` might be a subclass of FieldTextInput if that class doesn't // override the static fromJson method. diff --git a/tests/mocha/field_registry_test.js b/tests/mocha/field_registry_test.js index f8e4bb9b4..aca548746 100644 --- a/tests/mocha/field_registry_test.js +++ b/tests/mocha/field_registry_test.js @@ -42,12 +42,10 @@ suite('Field Registry', function () { }, 'Invalid name'); }); test('No fromJson', function () { - const fromJson = CustomFieldType.fromJson; - delete CustomFieldType.fromJson; + class IncorrectField {} chai.assert.throws(function () { - Blockly.fieldRegistry.register('field_custom_test', CustomFieldType); + Blockly.fieldRegistry.register('field_custom_test', IncorrectField); }, 'must have a fromJson function'); - CustomFieldType.fromJson = fromJson; }); test('fromJson not a function', function () { const fromJson = CustomFieldType.fromJson; @@ -97,5 +95,21 @@ suite('Field Registry', function () { chai.assert.isNotNull(field); chai.assert.equal(field.getValue(), 'ok'); }); + test('Did not override fromJson', function () { + // This class will have a fromJson method, so it can be registered + // but it doesn't override the abstract class's method so it throws + class IncorrectField extends Blockly.Field {} + + Blockly.fieldRegistry.register('field_custom_test', IncorrectField); + + const json = { + type: 'field_custom_test', + value: 'ok', + }; + + chai.assert.throws(function () { + Blockly.fieldRegistry.fromJson(json); + }, 'Attempted to instantiate a field from the registry'); + }); }); });