refactor(blocks): Migrate blocks/loops.js to TypeScript
#6957
Conversation
rachel-fenichel
requested review from
BeksOmega and
cpcallen
and removed request for
BeksOmega
There was a problem hiding this comment.
LGTM except for a few @Directives that should be removed. A few additional comments with queries for my curiosity.
blocks/loops.ts
Outdated
| */ | ||
|
|
||
| import * as goog from '../closure/goog/goog.js'; | ||
| goog.declareModuleId('Blockly.libraryBlocks.loops'); | ||
|
|
||
| /* eslint-disable-next-line no-unused-vars */ | ||
| import type * as AbstractEvent from '../core/events/events_abstract.js'; | ||
| import type {Abstract} from '../core/events/events_abstract.js'; |
There was a problem hiding this comment.
Two observations and an opinion:
- Interesting that Closure Compiler did not object to the previous (broken) import—presumably because of the
@suppress {checkTypes}. Abstractis more popular in our codebase thanAbstractEvent, but both appear.- I prefer
AbstractEventand think we should consider making it house style to always rename event imports toFooEventfor clarity.
(But not specifically requesting a change here.)
blocks/loops.ts
Outdated
| /** | ||
| * Mixin to add a context menu item to create a 'variables_get' block. | ||
| * Used by blocks 'controls_for' and 'controls_forEach'. | ||
| * | ||
| * @mixin |
There was a problem hiding this comment.
I have removed @mixin and @augments where I encountered them, as I had understood they were meaningless to our tooling (except Closure Compiler, but we aren't using it for type checking any more), and CUSTOM_CONTEXT_MENU_CREATE_VARIABLES_GET_MIXIN in any case is local and doesn't get actual generated documentation. Am I mistaken?
For properties/methods, @package should be replaced @internal—but this is a non-exported top-level declaration, so it not applicable here and should be deleted.
As far as @readonly: this should also be deleted. I had a look at the TSreadonly keyword, but it also applies to (individual) properties, not top-level declarations. I think the closest thing to @readonly here would be to declare const MY_MIXIN = { /* ... */ } as const, but I don't really think it's terribly useful since it seems unlikely we'd inadvertently write code that tries to modify properties of the mixin.
There was a problem hiding this comment.
Deleted mixin, augments, readonly, public and package annotations.
blocks/loops.ts
Outdated
| /** | ||
| * This mixin adds a check to make sure the 'controls_flow_statements' block | ||
| * is contained in a loop. Otherwise a warning is added to the block. | ||
| * | ||
| * @mixin |
blocks/loops.ts
Outdated
| @@ -243,9 +234,15 @@ Extensions.register( | |||
| 'controls_flow_tooltip', | |||
| Extensions.buildTooltipForDropdown('FLOW', BREAK_CONTINUE_TOOLTIPS)); | |||
|
|
|||
| /** Type of a block that has CUSTOM_CONTEXT_MENU_CREATE_VARIABLES_GET_MIXIN */ | |||
| type CustomContextMenuBlock = Block&CustomContextMenuMixin; | |||
| interface CustomContextMenuMixin extends CustomContextMenuMixinType {}; | |||
There was a problem hiding this comment.
Semicolon not needed:
| interface CustomContextMenuMixin extends CustomContextMenuMixinType {}; | |
| interface CustomContextMenuMixin extends CustomContextMenuMixinType {} |
blocks/loops.ts
Outdated
|
|
||
| /** Type of a block that has CONTROL_FLOW_IN_LOOP_CHECK_MIXIN */ | ||
| type ControlFlowInLoopBlock = Block&ControlFlowInLoopMixin; | ||
| interface ControlFlowInLoopMixin extends ControlFlowInLoopMixinType {}; |
There was a problem hiding this comment.
Semicolon not needed:
| interface ControlFlowInLoopMixin extends ControlFlowInLoopMixinType {}; | |
| interface ControlFlowInLoopMixin extends ControlFlowInLoopMixinType {} |
The basics
npm run formatandnpm run lintThe details
Resolves
Part of #6828
Proposed Changes
Migrates loop blocks, following the examples in #6900 and #6901