fix: improve types in FieldRegistry (#8062)

* fix: improve types in FieldRegistry

* chore: tsdoc

core/field.ts

@@ -1433,6 +1433,22 @@ export abstract class Field<T = any>
       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<typeof Field, 'prototype'>;
+type FieldProto = Pick<typeof Field, 'prototype'>;
 
 /**
  * Represents an error where the field is trying to access its block or

core/field_angle.ts

@@ -516,7 +516,7 @@ export class FieldAngle extends FieldInput<number> {
    * @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);

core/field_checkbox.ts

@@ -226,7 +226,9 @@ export class FieldCheckbox extends Field<CheckboxBool> {
    * @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);

core/field_colour.ts

@@ -641,7 +641,7 @@ export class FieldColour extends Field<string> {
    * @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);

core/field_dropdown.ts

@@ -647,7 +647,9 @@ export class FieldDropdown extends Field<string> {
    * @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 ' +

core/field_image.ts

@@ -250,7 +250,7 @@ export class FieldImage extends Field<string> {
    * @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' +

core/field_label.ts

@@ -117,7 +117,7 @@ export class FieldLabel extends Field<string> {
    * @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.

core/field_number.ts

@@ -315,7 +315,7 @@ export class FieldNumber extends FieldInput<number> {
    * @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(

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<T>(options: RegistryOptions): Field<T> | null {
  * @param options
  */
 function fromJsonInternal<T>(options: RegistryOptions): Field<T> | 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<T>(options: RegistryOptions): Field<T> | 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<T>;
-    return (fieldObject as unknown as {fromJson: fromJson}).fromJson(options);
   }
+  return fieldObject.fromJson(options);
 }
 
 export const TEST_ONLY = {

core/field_textinput.ts

@@ -73,7 +73,9 @@ export class FieldTextInput extends FieldInput<string> {
    * @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.

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');
+    });
   });
 });