fix: improve types in FieldRegistry

[maribethb]

The basics

• I validated my changes

The details

Resolves

Fixes #8059, to the extent that we're going to fix it anyway

Proposed Changes

• Creates & exports a new RegistrableField interface that represents a constructor for a Field class or subclass, and requires that constructed class also have a static fromJson method.
  • This type will be available as Blockly.fieldRegistry.RegistrableField.
  • This is exported because it is used as a parameter to the register function. Any functions in the public API should use and return accessible types, for the benefit of developers using TS. This solves the original issue reported by code.org.
  • See these docs for more information about this type
• Adjusted RegistryOptions type a bit.
  • Adds more comments to the RegistryOptions type. tbh this type is weird and needed more explanation.
  • Made the type actually be compatible with the type required by the field's fromJson method, because we are just passing this object directly into that function.
• Stopped exporting the FieldProto type from field.js
  • because we're only using it in that file now
  • We were never exporting this from the top level of blockly, so this doesn't change the public API
  • Added additional comments to explain what this is for so it doesn't get misused again.
• Added static fromJson to the base Field class
  • It doesn't return anything, it just throws an error if the method is called and was never overridden by a child class.
  • The method has to exist on any field that is registered so this gives us more accurate types then
  • This isn't a breaking change because either a custom field doesn't use the registry, in which case they'll never see the error, or it does use the registry and therefore already implements fromJson
• Adjusted the casts in the fromJson method of FieldRegistry.
  • Unfortunately casts are still required, as explained below. But I think this one is better.
  • Also removed the error that references an interface that doesn't exist.

Reason for Changes

Explained inline above.

Test Coverage

Added/fixed as necessary

Documentation

No

Additional Information

The types in the registry are still pretty wrong. The registry claims that what we'll get back from the getObject function is a Field. Unfortunately that's not correct. We'll get back a Field constructor, which as discussed in the docs also linked above, needs to be typed differently if we're actually referencing the constructor as a type.

The thing we get back from the getObject method should be a field constructor and it should have a fromJson method. This is the same exact type as what we pass into register, which makes sense. So we cast to that same type. Ideally the registry's generic types would have used these constructor interfaces instead of lying and saying they return Field instances. But fixing that is a bigger deal and would need to be fixed for other constructable objects, not just fields.

Also, the types of the fromJson method are also a bit sad, which is why the RegistryOptions type has to allow unknown keys. Each subclass of Field has its own fromJson method and this method takes its own FooConfigOptions type as a parameter, because each field has different things that need to be configured from json. If the Field class had the type of its fromJson method as a parameterized generic type, that would allow us to say "hey whatever Field subclass we're working with, it should call fromJson with its own FieldConfig type." Since we haven't done that, we can't type check the object passed to fromJson in the registry.

And, the RegistryOptions is a bit weird too. Your field's fromJson method better not expect a type property in the options object. Because we call the registry's fromJson method with only one parameter (which should have a type property) and then pass that same parameter directly into the field's fromJson method. That's a bit weird. I think ideally the field registry's fromJson method should have had type as a separate parameter, so that we aren't mixing in additional properties into the actual Field's config. But too late for that now.

[cpcallen]

LGTM modulo a couple of nits, but I'd recommend additional changes be made in v11 if possible:

• Stopped exporting the FieldProto type from field.js

  • because we're only using it in that file now
  • We were never exporting this from the top level of blockly, so this doesn't change the public API

I think this could be further tidied, with the remaining uses of FieldProto removed by changing the signatures of saveLegacyState and loadLegacyState to take just the needed methods needed (in a form such that the caller can pass the .prototype object:

protected saveLegacyState(prototype: `Pick<Field, 'saveState' | 'toXml'>): string | null
protected loadLegacyState(prototype: Pick<Field, 'loadState' | 'fromXml'>): boolean

The types in the registry are still pretty wrong. The registry claims that what we'll get back from the getObject function is a Field. Unfortunately that's not correct. We'll get back a Field constructor, which as discussed… needs to be typed differently if we're actually referencing the constructor as a type.

The thing we get back from the getObject method should be a field constructor and it should have a fromJson method. This is the same exact type as what we pass into register, which makes sense. So we cast to that same type. Ideally the registry's generic types would have used these constructor interfaces instead of lying and saying they return Field instances. But fixing that is a bigger deal and would need to be fixed for other constructable objects, not just fields.

Fixing that seems like a worthwhile thing to do. Arguably it could be postponed until after v11, though: if the signatures are really that wrong then fixing them won't break any correct programs, even if the fix itself is nominally a breaking change.

[cpcallen]

Line length.

[maribethb]

we are bikeshedding this in the team chat if you want to join.

[maribethb]

LGTM modulo a couple of nits, but I'd recommend additional changes be made in v11 if possible:

• Stopped exporting the FieldProto type from field.js

  • because we're only using it in that file now
  • We were never exporting this from the top level of blockly, so this doesn't change the public API

I think this could be further tidied, with the remaining uses of FieldProto removed by changing the signatures of saveLegacyState and loadLegacyState to take just the needed methods needed (in a form such that the caller can pass the .prototype object:

protected saveLegacyState(prototype: `Pick<Field, 'saveState' | 'toXml'>): string | null
protected loadLegacyState(prototype: Pick<Field, 'loadState' | 'fromXml'>): boolean

This would be a breaking change and I don't think it's worth doing. There's little if any benefit to the developer but there is cost. We could inline the existing FieldProto type directly into these two functions (which wouldn't be a breaking change) but I think there's some value in the comment explaining why the type exists and why typeof Field doesn't work.

The types in the registry are still pretty wrong. The registry claims that what we'll get back from the getObject function is a Field. Unfortunately that's not correct. We'll get back a Field constructor, which as discussed… needs to be typed differently if we're actually referencing the constructor as a type.
The thing we get back from the getObject method should be a field constructor and it should have a fromJson method. This is the same exact type as what we pass into register, which makes sense. So we cast to that same type. Ideally the registry's generic types would have used these constructor interfaces instead of lying and saying they return Field instances. But fixing that is a bigger deal and would need to be fixed for other constructable objects, not just fields.

Fixing that seems like a worthwhile thing to do. Arguably it could be postponed until after v11, though: if the signatures are really that wrong then fixing them won't break any correct programs, even if the fix itself is nominally a breaking change.

It's worthwhile in the abstract in that it would be nice if our types were not lies. But this is a much too big refactor to fit in before the v11 release. I also think it would be a breaking change, at least potentially, because developers may have worked around our incorrect types using a variety of methods that may not work anymore. And I'm not sure how important this particular ts refactor is compared to others that we know we need to do, like cleaning up our usage of @internal annotations. So I'm happy to file a bug with some of this context that we'll leave open, but it would need to be prioritized against everything else we plan to do and it's not going to make the cut for v11.