refactor(blocks): Migrate blocks/math.js to TypeScript
#6900
Conversation
This is just the result of running js2ts on this file.
cpcallen
added
component: library blocks
PR: chore
github-actions
bot
removed
the
PR: chore
blocks/math.ts
Outdated
| this.updateType_(newOp); | ||
| }.bind(this)); | ||
| const LIST_MODES_MUTATOR_EXTENSION = function(this: Block) { | ||
| this.getField('OP')!.setValidator(function(this: Field, newOp: string) { |
There was a problem hiding this comment.
Could the validator be converted to an arrow function to avoid having to explicitly bind this/generally modernize it?
There was a problem hiding this comment.
I don't think so, because the validator makes use of this, and if I understand correctly an arrow expression here would have this irrevocably bound to LIST_MODES_MUTATOR_EXTENSION, which is not what we want.
(If only block definitions were classes, and these could just be ordinary methods!)
There was a problem hiding this comment.
(No, now that I think more carefully I think that's wrong: this would be undefined—still wrong.)
There was a problem hiding this comment.
The more I look at this bit of code the weirder it is. I think I will rewrite it—but in a future PR. For now just try to preserve existing behaviour exactly.
Define types to represent the union of Block and each of the *_MIXINs, and use these types for the type of this in mixin methods.
Make sure validator functions explicitly return undefined. Field.prototype.setValidator takes a FieldValidator<T>, which must be a non-void function. I'm not sure why tsc was not objecting to the void implementation here, but it does in other very similar situations so for consistency explicitly return undefined to signal the value should be used unchanged. Also undo previous change of !. to ?.: I think it wisest to try to preserve the existing behaviour as exactly as possible for now, and make behaviour changes (i.e., ones that affect the generated code) separately.
|
I learned a bunch about how to more cleanly fix some type errors while working on |
It turns out that the previous types for these were completely wrong, for two reasons: - They should be intersection types (which have the union of the properties), not union types (which have the interseciton of the properties). - The *_MIXIN types were already declared as having type BlockDefinition, which is ultimately an alias for any. TypeScript doesn't like (some) kinds of circularly defined types, so fixing the above necessitates declaring a few auxiliary types just to make the type checker happy - but the end result is excellent and caught an actual type error in the code.
|
One more update: It turns out that the previous types for the mixins were completely wrong, for two reasons:
TypeScript doesn't like (some) kinds of circularly defined types, so fixing the above necessitates declaring a few auxiliary types just to make the type checker happy - but the end result is excellent and caught an actual type error in the code. |
|
So, I forgot to push commit 8591f44 before asking you to look again. Sorry. Do feel free to have a look at again, because the declaration of the types of the mixins is much more correct but also much less pleasant. |
The basics
npm run formatandnpm run lintThe details
Resolves
Part of #6828.
Additional Information
The types are not completely finalised, in particular because
BlockDefinitionis defined asAnyDuringMigration, and so it has been necessary in a few places to use.!and/oras AnyDuringMigration. Hopefully it will be possible to address these in a future PR.