fix: Correct enum formatting, use merged `namespace`s for types that are class static members #6246

cpcallen · 2022-06-26T22:52:36Z

The basics

I branched from ts/migration
My pull request is against ts/migration
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

Some of the damage done by MigranTS to our files—in particular causing types declared as static members of classes such as ContextMenuRegistry.* to no longer be exported from the top-level Blockly module (blockly.js/.ts) because they:
- were interfaces that had been converted to named exports and were no longer available via a static property at all, or
- were enums that had been reexported under their original static name by assignment, which exports the enum dictionary object but not the associated type information, meaning that (e.g.) ContextMenuRegistry.ScopeType could not be used as a type name.

Proposed Changes

Fix formatting issues for KeyCodes enum in core/utils/keycodes.ts.
Fix formatting and exporting of ContextMenuRegistry static types in core/contextmenu_registry.ts.
Fix formatting and exporting of Input.Align in core/input.ts.
Fix exporting of Names.NameType in core/names.ts.

Reason for Changes

Key codes actually matter—assuming sequential alignment is fragile in the face of additions/deletions!
Readable comments matter.
Maintaining backwards-compatibility matters.

Additional Information

The use of merged namespaces violates the Google TS style guide, which forbids namespace, but as far as I can tell this is the recommended way to model the common JavaScript pattern of types as static properties of a class constructor. I do not advocate for ignoring this rule entirely—only making an exception where we need to maintain backwards compatibility with our existing API and this is the suggested approach.

N.B: this PR does not address all the enum formatting issues that need correction, nor all of the static-member types that have been moved-and-therefore-deleted by MigranTs. It is instead intended mainly to be a model for (and opportunity to discuss the approach to) solving the issues noted.

@gonfunko

- Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.)

- Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export.

- Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.)

cpcallen · 2022-06-26T22:54:15Z

Reviews requested from @BeksOmega as she is the lead for the ts/migration branch, and additionally from @maribethb and @rachel-fenichel to solicit comments on the merged namespace approach.

BeksOmega · 2022-06-27T15:10:42Z

Important info from Christopher:

It looks like we need to export some of these static subtypes both as they have been (static properties on a class) for backwards compatibility, and also as named exports in their own right (as the auto converter has converted some of our code to expect them that way). This created a little problem for enum types, about which I asked on Stack Overflow (though in the process of asking I found an answer that at least works): https://stackoverflow.com/questions/72763491/how-to-correctly-alias-enum-types-and-their-objects-in-typescript/72763492#72763492

BeksOmega

Overall approach looks good to me.

I do not want to export these enums from blockly.js/ts until we're done with the rest of the migration. Because I don't think we have time to really consider how we want people to access things right now.

core/contextmenu_registry.ts

core/input.ts

core/names.ts

BeksOmega · 2022-06-27T20:25:31Z

I'm working on converting the typedefs to follow this approach.

[Edit: And now the other enums as well]

cpcallen · 2022-06-28T18:23:53Z

I do not want to export these enums from blockly.js/ts until we're done with the rest of the migration. Because I don't think we have time to really consider how we want people to access things right now.

Do you mean that you don't want to add, new separate reexports of these types to blockly.js/ts (at least for the moment)?
If so I absolutely agree.

But otherwise I'm confused about your question because we were already exporting them via static properties on their respective constructor functions, and the point of this PR is to ensure we continue to do so.

- Use `Align` instead of `Input.Align` where possible.

BeksOmega · 2022-06-28T19:33:44Z

I do not want to export these enums from blockly.js/ts until we're done with the rest of the migration. Because I don't think we have time to really consider how we want people to access things right now.

Do you mean that you don't want to add, new separate reexports of these types to blockly.js/ts (at least for the moment)? If so I absolutely agree.

But otherwise I'm confused about your question because we were already exporting them via static properties on their respective constructor functions, and the point of this PR is to ensure we continue to do so.

I mean I don't want to add any new ways that external developers can access these properties - at least not right now. I want them to continue accessing them as static properties on the class. I don't want them to be accessible via the module.

I think we're actually on the same page there. Because right now in blockly.js/ts we're using destructuring imports to just get the class right? So unless we modify blockly.js/ts, the only way to access the enums is as static properties on the class. I might have been confused about this when I posted originally.

But then my question is, do we need the export const E = C.E; line if we don't plan to modify blockly.js/ts to export these from the top level?

[Edit: Ok, because it is both a value and a type, we have to export it both ways from the module, so that the other internal places where migrants has converted things to access the enum via the module continue to work (both when accessing the enum as a type, and as a value). Is that correct?]

BeksOmega

Going to approve based on the assumption that my new understanding is correct haha, so that you (@cpcallen ) can merge whenever you have time.

rachel-fenichel

This approach LGTM, although I understand the concerns about namespace merging.

cpcallen · 2022-06-28T21:44:37Z

I mean I don't want to add any new ways that external developers can access these properties - at least not right now. I want them to continue accessing them as static properties on the class.

Agreed.

So unless we modify blockly.js/ts, the only way to access the enums is as static properties on the class.

Correct.

But then my question is, do we need the export const E = C.E; line if we don't plan to modify blockly.js/ts to export these from the top level?

Unfortunately MigranTS seems to have (in many cases, including IIRC these ones) converted other modules to use the named export rather than the static property, so they need to "be accessible via the module" unless/until we go and undo those conversions.

Going to approve based on the assumption that my new understanding is correct haha, so that you (@cpcallen ) can merge whenever you have time.

Thanks. I did a merge of ts/migration into this branch locally to make sure it resolved all the tsc errors, and while it did it caused me to notice that it was producing new Closure Compiler JSC_UNUSED_LOCAL_ASSIGNMENT errors which I should address before submitting. Alas: a task for tomorrow.

tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files.

@gonfunko

…are class static members (google#6246) * fix: formatting of enum KeyCodes * fix: Use merged namespace for ContextMenuRegistry static types - Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.) * fix: Use merged namespace for Input.Align - Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export. * fix: Use merged namespace for Names.NameType - Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.) * chore: Fix minor issues for PR google#6246 - Use `Align` instead of `Input.Align` where possible. * fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files.

@gonfunko

…are class static members (google#6246) * fix: formatting of enum KeyCodes * fix: Use merged namespace for ContextMenuRegistry static types - Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.) * fix: Use merged namespace for Input.Align - Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export. * fix: Use merged namespace for Names.NameType - Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.) * chore: Fix minor issues for PR google#6246 - Use `Align` instead of `Input.Align` where possible. * fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files.

@gonfunko

…are class static members (google#6246) * fix: formatting of enum KeyCodes * fix: Use merged namespace for ContextMenuRegistry static types - Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.) * fix: Use merged namespace for Input.Align - Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export. * fix: Use merged namespace for Names.NameType - Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.) * chore: Fix minor issues for PR google#6246 - Use `Align` instead of `Input.Align` where possible. * fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files.

@gonfunko

* fix: convert files to typescript * fix: add alias for AnyDuringMigration so that tsc will run * chore: format * chore: enable ts for the clang-format workflow (#6233) * chore: Restore @fileoverview comment locations (#6237) * chore: add declareModuleId (#6238) * fix: Revert comment change to app_controller.js (#6241) * fix: Add missing import goog statements (#6240) I've added the import statement immediately before the goog.declareModuleId calls that depend on it. There is an argument to be made that we should put the import statement in their normal place amongst any other imports, and move the declareModuleId statement to below the double blank line below the imports, but as these are so tightly coupled, replace the previous goog.module calls, and will both be deleted at the same time once the transition to TypeScript is fully complete I think it's fine (and certainly much easier) to do it this way. * chore: Fix whitespace (#6243) * fix: Remove spurious blank lines Remove extraneous blank lines introduced by deletion of 'use strict'; pragmas. Also fix the location of the goog.declareModuleId call in core/utils/array.ts. * fix: Add missing double-blank-line before body of modules Our convention is to have two blank lines between the imports (or module ID, if there are no imports) and the beginning of the body of the module. Enforce this. * fix: one addition format error for PR #6243 * fix(build): Skip npm prepare when running in CI (#6244) Have npm prepare do nothing when running in CI. We don't need to do any building, because npm test will build everything needed in the workflows in which it is run, and we don't want to build anything in other workflows because a tsc error would prevent those workflows from completing. * fix: re-add `@package` annotations as `@internal` annotations (#6232) * fix: add ~70% of internal attributes * fix: work on manually adding more @internal annotations * fix: add more manual internal annotations * fix: rename package typos to internal * fix: final manual fixes for internal annotations * chore: format * chore: make unnecessary multiline jsdoc a single line * fix: fix internal tags in serialization exceptions * fix: tsc errors picked up from develop (#6224) * fix: relative path for deprecation utils * fix: checking if properties exist in svg_math * fix: set all timeout PIDs to AnyDuringMigration * fix: make nullability errors explicity in block drag surface * fix: make null check in events_block_change explicit * fix: make getEventWorkspace_ internal so we can access it from CommentCreateDeleteHelper * fix: rename DIV -> containerDiv in tooltip * fix: ignore backwards compat check in category * fix: set block styles to AnyDuringMigration * fix: type typo in KeyboardShortcut * fix: constants name in row measurables * fix: typecast in mutator * fix: populateProcedures type of flattened array * fix: ignore errors related to workspace comment deserialization * chore: format files * fix: renaming imports missing file extensions * fix: remove check for sound.play * fix: temporarily remove bad requireType. All `export type` statements are stripped when tsc is run. This means that when we attempt to require BlockDefinition from the block files, we get an error because it does not exist. We decided to temporarily remove the require, because this will no longer be a problem when we conver the blocks to typescript, and everything gets compiled together. * fix: bad jsdoc in array * fix: silence missing property errors Closure was complaining about inexistant properties, but they actually do exist, they're just not being transpiled by tsc in a way that closure understands. I.E. if things are initialized in a function called by the constructor, rather than in a class field or in the custructor itself, closure would error. It would also error on enums, because they are transpiled to a weird IIFE. * fix: context menu action handler not knowing the type of this. this: TypeX information gets stripped when tsc is run, so closure could not know that this was not global. Fixed this by reorganizing to use the option object directly instead of passing it to onAction to be bound to this. * fix: readd getDeveloperVars checks (should not be part of migration) This was found because ALL_DEVELOPER_VARS_WARNINGS_BY_BLOCK_TYPE was no longer being accessed. * fix: silence closure errors about overriding supertype props We propertly define the overrides in typescript, but these get removed from the compiled output, so closure doesn't know they exist. * fix: silence globalThis errors this: TypeX annotations get stripped from the compiled output, so closure can't know that we're accessing the correct things. However, typescript makes sure that this always has the correct properties, so silencing this should be fine. * fix: bad jsdoc name * chore: attempt compiling with blockly.js * fix: attempt moving the import statement above the namespace line * chore: add todo comments to block def files * chore: remove todo from context menu * chore: add comments abotu disabled errors * chore: move comments back to their correct positions (#6249) * fix: work on fixing comments * chore: finish moving all comments * chore: format * chore: move some other messed up comments * chore: format * fix: Correct enum formatting, use merged `namespace`s for types that are class static members (#6246) * fix: formatting of enum KeyCodes * fix: Use merged namespace for ContextMenuRegistry static types - Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.) * fix: Use merged namespace for Input.Align - Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export. * fix: Use merged namespace for Names.NameType - Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.) * chore: Fix minor issues for PR #6246 - Use `Align` instead of `Input.Align` where possible. * fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files. * chore(build): Suppress spurious warnings from closure-make-deps (#6253) A little bit of an ugly hack, but it works: pipe stderr through grep -v to suppress error output starting with "WARNING in". * fix: remaining enums that weren't properly exported (#6251) * fix: remaining enums that weren't properly exported * chore: format * fix: add enum value exports * chore: format * fix: properly export interfaces that were typedefs (#6250) * fix: properly export interfaces that were typedefs * fix: allowCollsion -> allowCollision * fix: convert unconverted enums * fix: enums that were/are instance properties * fix: revert changes to property enums * fix: renamed protected parameter properties (#6252) * fix: bad protected parameter properties * chore:format * fix: gesture constructor * fix: overridden properties that were renamed * refactor: Migrate `blockly.js` to TypeScript (#6261) * chore: Apply changes to blockly.js to blockly.ts * fix: Build using core/blockly.ts instead of .js Compiles and runs in compressed mode correctly! * fix(build): Don't depend on execSync running bash (#6262) For some reason on Github CI servers execSync uses /bin/sh, which is (on Ubuntu) dash rather than bash, and does not understand the pipefail option. So remove the grep pipe on stderr and just discard all error output at all. This is not ideal as errors in test deps will go unreported AND not even cause test failure, but it's not clear that it's worth investing more time to fix this at the moment. * chore: use `import type` where possible (#6279) * chore: automatically change imports to import types * chore: revert changes that actually need to be imports * chore: format * chore: add more import type statements based on importsNotUsedAsValues * chore: fix tsconfig * chore: add link to compiler issue * fix: add type information to blockly options (#6283) * fix: add type information to blockly options * chore: format * chore: remove erroneous comment * fix: bugs revealed by getting the built output working (#6282) * fix: types of compose and decompose in block * fix: workspace naming in toolbox * chore: add jsdoc * chore: restore registry comments to better positions * chore: pr comments' * fix(variables): Revert inadvertent change to allDeveloperVariables (#6290) It appears that a function call got modified incorrectly (probably in an effort to fix a typing issue). This fix trivially reverts the line in question to match the original JS version from develop. This causes the generator tests to pass. * fix: circular dependencies (#6281) * chore: fix circular dependencies w/ static workspace funcs * remove preserved imports that aren't currently necessary (probably) * fix circular dependency with workspaces and block using stub * fix dependency between variables and xml by moving function to utils * add stub for trashcan as well * fix line endings from rebase * fix goog/base order * add trashcan patch * fix: types of compose and decompose in block * fix: workspace naming in toolbox * chore: add jsdoc * chore: restore registry comments to better positions * chore: remove implementations in goog.js * chore: fix types of stubs * chore: remove added AnyDuringMigration casts * chore: remove modifications to xml and variables * chore: format * chore: remove event requirements in workspace comments * chore: fix circular dependency with xml and workspace comments * fixup remove ContextMenu import * chore: fix dependency between mutator and workspace * chore: break circular dependency between names and procedures * chore: get tests to run? * chore: pr comments' * chore: fix stubbing field registry fromJson * chore: fix spying on fire * chore: fix stubbing parts of connection checker * chore: fix stubbing dialog * chore: fix stubbing style * chore: fix spying on duplicate * chore: fix stubbing variables * chore: fix stubbing copy * chore: fix stubbing in workspace * chore: remove unnecessary stubs * chore: fix formatting * chore: fix other formatting * chore: add backwards compatible static properties to workspace * chore: move static type properties * chore: move and comment stubs * chore: add newlines at EOF * chore: improve errors for monkey patched functions * chore: update comment with a pointer to the doc * chore: update comment with a pointer to the doc * chore: format * chore: revert changes to playground used for testing (#6292) * chore: get mocha tests to pass. (#6291) * chore: fix undo and empty code blocks * chore: skip IE test * chore: fix gesture test * chore: fix replace message references test * chore: fix string table interpolation * chore: skip getById tests * chore: fix field tests * chore: fix console errors by making workspace nullable * chore: format * chore: fix definition overwrite warning * chore: update metadata * chore: temporarily modify the the advanced compile test * chore: fix gestures by fixing test instead Co-authored-by: Neil Fraser <fraser@google.com> Co-authored-by: Christopher Allen <cpcallen+git@google.com>

@gonfunko

fix: add alias for AnyDuringMigration so that tsc will run chore: format chore: enable ts for the clang-format workflow (google#6233) chore: Restore @fileoverview comment locations (google#6237) chore: add declareModuleId (google#6238) fix: Revert comment change to app_controller.js (google#6241) fix: Add missing import goog statements (google#6240) I've added the import statement immediately before the goog.declareModuleId calls that depend on it. There is an argument to be made that we should put the import statement in their normal place amongst any other imports, and move the declareModuleId statement to below the double blank line below the imports, but as these are so tightly coupled, replace the previous goog.module calls, and will both be deleted at the same time once the transition to TypeScript is fully complete I think it's fine (and certainly much easier) to do it this way. chore: Fix whitespace (google#6243) * fix: Remove spurious blank lines Remove extraneous blank lines introduced by deletion of 'use strict'; pragmas. Also fix the location of the goog.declareModuleId call in core/utils/array.ts. * fix: Add missing double-blank-line before body of modules Our convention is to have two blank lines between the imports (or module ID, if there are no imports) and the beginning of the body of the module. Enforce this. * fix: one addition format error for PR google#6243 fix(build): Skip npm prepare when running in CI (google#6244) Have npm prepare do nothing when running in CI. We don't need to do any building, because npm test will build everything needed in the workflows in which it is run, and we don't want to build anything in other workflows because a tsc error would prevent those workflows from completing. fix: re-add `@package` annotations as `@internal` annotations (google#6232) * fix: add ~70% of internal attributes * fix: work on manually adding more @internal annotations * fix: add more manual internal annotations * fix: rename package typos to internal * fix: final manual fixes for internal annotations * chore: format * chore: make unnecessary multiline jsdoc a single line * fix: fix internal tags in serialization exceptions fix: tsc errors picked up from develop (google#6224) * fix: relative path for deprecation utils * fix: checking if properties exist in svg_math * fix: set all timeout PIDs to AnyDuringMigration * fix: make nullability errors explicity in block drag surface * fix: make null check in events_block_change explicit * fix: make getEventWorkspace_ internal so we can access it from CommentCreateDeleteHelper * fix: rename DIV -> containerDiv in tooltip * fix: ignore backwards compat check in category * fix: set block styles to AnyDuringMigration * fix: type typo in KeyboardShortcut * fix: constants name in row measurables * fix: typecast in mutator * fix: populateProcedures type of flattened array * fix: ignore errors related to workspace comment deserialization * chore: format files * fix: renaming imports missing file extensions * fix: remove check for sound.play * fix: temporarily remove bad requireType. All `export type` statements are stripped when tsc is run. This means that when we attempt to require BlockDefinition from the block files, we get an error because it does not exist. We decided to temporarily remove the require, because this will no longer be a problem when we conver the blocks to typescript, and everything gets compiled together. * fix: bad jsdoc in array * fix: silence missing property errors Closure was complaining about inexistant properties, but they actually do exist, they're just not being transpiled by tsc in a way that closure understands. I.E. if things are initialized in a function called by the constructor, rather than in a class field or in the custructor itself, closure would error. It would also error on enums, because they are transpiled to a weird IIFE. * fix: context menu action handler not knowing the type of this. this: TypeX information gets stripped when tsc is run, so closure could not know that this was not global. Fixed this by reorganizing to use the option object directly instead of passing it to onAction to be bound to this. * fix: readd getDeveloperVars checks (should not be part of migration) This was found because ALL_DEVELOPER_VARS_WARNINGS_BY_BLOCK_TYPE was no longer being accessed. * fix: silence closure errors about overriding supertype props We propertly define the overrides in typescript, but these get removed from the compiled output, so closure doesn't know they exist. * fix: silence globalThis errors this: TypeX annotations get stripped from the compiled output, so closure can't know that we're accessing the correct things. However, typescript makes sure that this always has the correct properties, so silencing this should be fine. * fix: bad jsdoc name * chore: attempt compiling with blockly.js * fix: attempt moving the import statement above the namespace line * chore: add todo comments to block def files * chore: remove todo from context menu * chore: add comments abotu disabled errors chore: move comments back to their correct positions (google#6249) * fix: work on fixing comments * chore: finish moving all comments * chore: format * chore: move some other messed up comments * chore: format fix: Correct enum formatting, use merged `namespace`s for types that are class static members (google#6246) * fix: formatting of enum KeyCodes * fix: Use merged namespace for ContextMenuRegistry static types - Create a namespace to be merged with the ContextMenuRegistry class containing the types that were formerly declared as static properties on that class. - Use type aliases to export them individually as well, for compatibility with the changes made by MigranTS (and/or @gonfunko) to how other modules in core/ now import these types. - Update renamings.json5 to reflect the availability of the direct exports for modules that import this module directly (though they are not available to, and will not be used by, code that imports only via blockly.js/blockly.ts.) * fix: Use merged namespace for Input.Align - Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export. * fix: Use merged namespace for Names.NameType - Create a merged namespace for the Names.NameType enum. - Use type/const aliases to export it as NameType too. - Update renamings.json5 to reflect the availability of the direct export. (This ought to have happened in an earlier version as it was already available by both routes.) * chore: Fix minor issues for PR google#6246 - Use `Align` instead of `Input.Align` where possible. * fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors tsc generates code for merged namespaces that looks like: (function (ClassName) { let EnumName; (function (EnumName) { EnumName[EnumNameAlign["v1"] = 0] = "v1"; // etc. })(EnumName = ClassName.EnumName || (ClassName.EnumName = {})); })(ClassName || (ClassName = {})); and Closure Compiler complains about the fact that the EnumName let binding is initialised but never used. (It exists so that any other code that was in the namespace could see the enum.) Suppress this message, since it is not actionable and lint and/or tsc should tell us if we have actual unused variables in our .ts files. chore(build): Suppress spurious warnings from closure-make-deps (google#6253) A little bit of an ugly hack, but it works: pipe stderr through grep -v to suppress error output starting with "WARNING in". fix: remaining enums that weren't properly exported (google#6251) * fix: remaining enums that weren't properly exported * chore: format * fix: add enum value exports * chore: format fix: properly export interfaces that were typedefs (google#6250) * fix: properly export interfaces that were typedefs * fix: allowCollsion -> allowCollision * fix: convert unconverted enums * fix: enums that were/are instance properties * fix: revert changes to property enums fix: renamed protected parameter properties (google#6252) * fix: bad protected parameter properties * chore:format * fix: gesture constructor * fix: overridden properties that were renamed refactor: Migrate `blockly.js` to TypeScript (google#6261) * chore: Apply changes to blockly.js to blockly.ts * fix: Build using core/blockly.ts instead of .js Compiles and runs in compressed mode correctly! fix(build): Don't depend on execSync running bash (google#6262) For some reason on Github CI servers execSync uses /bin/sh, which is (on Ubuntu) dash rather than bash, and does not understand the pipefail option. So remove the grep pipe on stderr and just discard all error output at all. This is not ideal as errors in test deps will go unreported AND not even cause test failure, but it's not clear that it's worth investing more time to fix this at the moment. chore: use `import type` where possible (google#6279) * chore: automatically change imports to import types * chore: revert changes that actually need to be imports * chore: format * chore: add more import type statements based on importsNotUsedAsValues * chore: fix tsconfig * chore: add link to compiler issue fix: add type information to blockly options (google#6283) * fix: add type information to blockly options * chore: format * chore: remove erroneous comment fix: bugs revealed by getting the built output working (google#6282) * fix: types of compose and decompose in block * fix: workspace naming in toolbox * chore: add jsdoc * chore: restore registry comments to better positions * chore: pr comments' fix(variables): Revert inadvertent change to allDeveloperVariables (google#6290) It appears that a function call got modified incorrectly (probably in an effort to fix a typing issue). This fix trivially reverts the line in question to match the original JS version from develop. This causes the generator tests to pass. fix: circular dependencies (google#6281) * chore: fix circular dependencies w/ static workspace funcs * remove preserved imports that aren't currently necessary (probably) * fix circular dependency with workspaces and block using stub * fix dependency between variables and xml by moving function to utils * add stub for trashcan as well * fix line endings from rebase * fix goog/base order * add trashcan patch * fix: types of compose and decompose in block * fix: workspace naming in toolbox * chore: add jsdoc * chore: restore registry comments to better positions * chore: remove implementations in goog.js * chore: fix types of stubs * chore: remove added AnyDuringMigration casts * chore: remove modifications to xml and variables * chore: format * chore: remove event requirements in workspace comments * chore: fix circular dependency with xml and workspace comments * fixup remove ContextMenu import * chore: fix dependency between mutator and workspace * chore: break circular dependency between names and procedures * chore: get tests to run? * chore: pr comments' * chore: fix stubbing field registry fromJson * chore: fix spying on fire * chore: fix stubbing parts of connection checker * chore: fix stubbing dialog * chore: fix stubbing style * chore: fix spying on duplicate * chore: fix stubbing variables * chore: fix stubbing copy * chore: fix stubbing in workspace * chore: remove unnecessary stubs * chore: fix formatting * chore: fix other formatting * chore: add backwards compatible static properties to workspace * chore: move static type properties * chore: move and comment stubs * chore: add newlines at EOF * chore: improve errors for monkey patched functions * chore: update comment with a pointer to the doc * chore: update comment with a pointer to the doc * chore: format chore: revert changes to playground used for testing (google#6292) chore: get mocha tests to pass. (google#6291) * chore: fix undo and empty code blocks * chore: skip IE test * chore: fix gesture test * chore: fix replace message references test * chore: fix string table interpolation * chore: skip getById tests * chore: fix field tests * chore: fix console errors by making workspace nullable * chore: format * chore: fix definition overwrite warning * chore: update metadata * chore: temporarily modify the the advanced compile test * chore: fix gestures by fixing test instead

cpcallen added 4 commits June 24, 2022 23:30

fix: formatting of enum KeyCodes

b2c6c3f

fix: Use merged namespace for Input.Align

b19472e

- Create a merged namespace for the Input.Align enum. - Use type/const aliases to export it as Input too. - Update renamings.json5 to reflect the availability of the direct export.

cpcallen added component: TypeScript PR: fix Fixes a bug labels Jun 26, 2022

cpcallen requested review from maribethb, rachel-fenichel and BeksOmega June 26, 2022 22:52

cpcallen requested a review from a team as a code owner June 26, 2022 22:52

github-actions bot assigned BeksOmega, maribethb and rachel-fenichel Jun 26, 2022

BeksOmega reviewed Jun 27, 2022

View reviewed changes

core/contextmenu_registry.ts Show resolved Hide resolved

core/input.ts Show resolved Hide resolved

core/names.ts Show resolved Hide resolved

This was referenced Jun 27, 2022

fix: properly export interfaces that were typedefs #6250

Merged

fix: remaining enums that weren't properly exported #6251

Merged

cpcallen added 2 commits June 28, 2022 20:00

chore: Fix minor issues for PR google#6246

36cc57d

- Use `Align` instead of `Input.Align` where possible.

Merge branch 'ts/migration' into ts/types

0309a37

BeksOmega approved these changes Jun 28, 2022

View reviewed changes

rachel-fenichel approved these changes Jun 28, 2022

View reviewed changes

cpcallen merged commit 9eeb4fe into google:ts/migration Jun 29, 2022

cpcallen deleted the ts/types branch June 29, 2022 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Correct enum formatting, use merged `namespace`s for types that are class static members #6246

fix: Correct enum formatting, use merged `namespace`s for types that are class static members #6246

cpcallen commented Jun 26, 2022

cpcallen commented Jun 26, 2022

BeksOmega commented Jun 27, 2022 •

edited

Loading

BeksOmega left a comment

BeksOmega commented Jun 27, 2022 •

edited

Loading

cpcallen commented Jun 28, 2022

BeksOmega commented Jun 28, 2022 •

edited

Loading

BeksOmega left a comment

rachel-fenichel left a comment

cpcallen commented Jun 28, 2022

fix: Correct enum formatting, use merged namespaces for types that are class static members #6246

fix: Correct enum formatting, use merged namespaces for types that are class static members #6246

Conversation

cpcallen commented Jun 26, 2022

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Additional Information

cpcallen commented Jun 26, 2022

BeksOmega commented Jun 27, 2022 • edited Loading

BeksOmega left a comment

Choose a reason for hiding this comment

BeksOmega commented Jun 27, 2022 • edited Loading

cpcallen commented Jun 28, 2022

BeksOmega commented Jun 28, 2022 • edited Loading

BeksOmega left a comment

Choose a reason for hiding this comment

rachel-fenichel left a comment

Choose a reason for hiding this comment

cpcallen commented Jun 28, 2022

fix: Correct enum formatting, use merged `namespace`s for types that are class static members #6246

fix: Correct enum formatting, use merged `namespace`s for types that are class static members #6246

BeksOmega commented Jun 27, 2022 •

edited

Loading

BeksOmega commented Jun 27, 2022 •

edited

Loading

BeksOmega commented Jun 28, 2022 •

edited

Loading