fix: circular dependencies #6281

BeksOmega · 2022-07-22T19:02:51Z

The basics

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

The details

Resolves

Built code not actually working, due to circular dependencies.

Proposed Changes

Does various things to make the built code actually work.

Moves getById and getAll into common to break some dependency cycles.
Comments out imports of certain things being used for side effects to break dependency cycles. Basically I commented them out until it started working.
Creates a bunch of "stub" functions/methods that do not have implementations in the file they belong to (so that we can break dependency cycles). The implementations are monkey-patched in in blockly.ts
Creates internal versions of a bunch of functions that are added to the TEST_ONLY export of their respective modules so that they can be spied/stubbed in tests.
Changes goog.js to include actual definitions of its functions, instead of pointing to the global goog and assuming it exists. The closure compiler couldn't figure out how to order things to get this to work. The definitions in goog.js just call the global goog.
Commented out the declareModuleId error we were getting in uncompiled mode (using the debug module loader), so that we can run tests and the playground.

Behavior Before Change

Built code did not work.

Behavior After Change

Built code works!

Reason for Changes

Things working is good.

Test Coverage

Documentation

N/A

Additional Information

This is super messy because I was just trying to get things to work. Happy to clean up later, but wanted to put this up for discussion. Also happy to answer any questions about the weird things I am doing.

Future work

Investigate whether those commented-out imports are necessary or not.
Get rid of any internal functions that were created for test stubbing.
Figure out a way to run tests without commenting out the declareModuleId error. Eg use webpack or ESModule loading or something.

cpcallen · 2022-07-25T15:53:50Z

closure/goog/goog.js

+export const require = function(namespace) {
+  if (!COMPILED) {
+    // Might need to lazy load on old IE.
+    if (goog.ENABLE_DEBUG_LOADER) {
+      goog.debugLoader_.requested(namespace);
+    }
+
+    // If the object already exists we do not need to do anything.
+    if (goog.isProvided_(namespace)) {
+      if (goog.isInModuleLoader_()) {
+        return goog.module.getInternal_(namespace);
+      }
+    } else if (goog.ENABLE_DEBUG_LOADER) {
+      var moduleLoaderState = goog.moduleLoaderState_;
+      goog.moduleLoaderState_ = null;
+      try {
+        goog.debugLoader_.load_(namespace);
+      } finally {
+        goog.moduleLoaderState_ = moduleLoaderState;
+      }
+    }
+
+    return null;
+  }
+};


Is goog.require used in any of our .ts files? Surely this can be deleted (or at least: the former definition commented out) entirely?

I don't think we use it in any of our TS files, but we do use it in the blocks and the generators, which are also compiled using the goog.js file. SO I think we need to keep it? But I'll give removing it a shot as well.

Oh I guess the thing is the blocks use the global goog? Not the one imported from the goog.js module?

Correct: blocks and generators have not yet been migrated to ES/TS modules, so do not use goog.js, only the versions defined in base.js.

closure/goog/goog.js

core/procedures.ts

core/utils/xml.ts

core/workspace_svg.ts

core/procedures.ts

core/variables.ts

core/utils/xml.ts

cpcallen

Approved but with nits.

core/utils/xml.ts

core/blockly.ts

cpcallen · 2022-07-25T23:06:25Z

I will leave it to your judgement about whether you think this or the goog.module.get approach is the more promising one to go with.

maribethb

Overall thoughts

I don't love the patching approach, but it looks like that is what is going to unblock the circular dependency problem and allow us a path forward, so I can get over it.
I strongly dislike the TEST_ONLY solution to the mocking problem :( If this is the best path for unblocking us, then I can get over it for now but I really don't want this to stick around. I looked into it and it looks like there are some solutions for mocking modules like proxyquire, quibble, etc that allow us to basically sub in a fake module that we can stub. Alternatively I think if the tests used the compiled code we wouldn't run into this issue? Or some tests could be rearchitected to not have to stub as many things. Or if we make bigger changes to Blockly's architecture to fix some of the dependency cycles then we could also make some changes to allow for easier testing. I think in the near future after this is landed we should devote some time to really dig into current best practices for tests and improve our story on that front.
Can you update the PR description to add more information, since in the future this is likely where we'll trace these changes to. This can just be a link to the doc we have about work needed to unblock us, as long as that doc is up to date. Basically can you list the changes you had to make in this PR, and ideally give some indication as to whether there is any follow up work needed? For example as a follow up we should remove all of the dependencies that you've commented out here.

So I'm approving these changes, on the basis that they are the least-bad option we currently have, and acknowledging that some of these changes need follow up work.

core/blockly.ts

core/clipboard.ts

core/mutator.ts

cpcallen

I would put the …Internal implementations directly after the original function they have hollowed out. It's more readable, and makes for a cleaner diff.

(Unless you think the original function was in the wrong place, in which case move it to wherever you have put the …Internal verison.

core/dialog.ts

core/utils/style.ts

cpcallen

Whoop whoop!

cpcallen · 2022-07-29T10:45:57Z

tests/bootstrap.js

+      'build/blockly_compressed.js',
+      'build/blocks_compressed.js',
+      'build/dart_compressed.js',
+      'build/javascript_compressed.js',
+      'build/lua_compressed.js',
+      'build/php_compressed.js',
+      'build/python_compressed.js',


Did you see #6218, and the reason it is still a draft PR and not merged yet?

Yeah this was committed accidentally :/ will revert.

* 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

@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

BeksOmega added 10 commits July 21, 2022 22:27

chore: fix circular dependencies w/ static workspace funcs

4d4e847

remove preserved imports that aren't currently necessary (probably)

7ebe9eb

fix circular dependency with workspaces and block using stub

c1c37d1

fix dependency between variables and xml by moving function to utils

cb6f688

add stub for trashcan as well

1907e80

fix line endings from rebase

beb1521

fix goog/base order

2065b5d

add trashcan patch

fa18faa

fix: types of compose and decompose in block

2dfd41a

fix: workspace naming in toolbox

1678ebc

BeksOmega requested a review from a team as a code owner July 22, 2022 19:02

BeksOmega requested review from gonfunko and removed request for a team July 22, 2022 19:02

github-actions bot assigned gonfunko Jul 22, 2022

BeksOmega added 2 commits July 22, 2022 22:13

chore: add jsdoc

7c32766

chore: restore registry comments to better positions

5eaf042

BeksOmega requested review from cpcallen and removed request for gonfunko July 25, 2022 15:12

BeksOmega assigned cpcallen and unassigned gonfunko Jul 25, 2022

cpcallen reviewed Jul 25, 2022

View reviewed changes

BeksOmega added 5 commits July 25, 2022 18:57

chore: remove implementations in goog.js

090cd0a

chore: fix types of stubs

e0cc6e0

chore: remove added AnyDuringMigration casts

b205d87

chore: remove modifications to xml and variables

28fb935

chore: format

ba997ca

cpcallen approved these changes Jul 25, 2022

View reviewed changes

core/utils/xml.ts Outdated Show resolved Hide resolved

core/blockly.ts Outdated Show resolved Hide resolved

BeksOmega added 2 commits July 26, 2022 21:42

chore: remove event requirements in workspace comments

03d1402

chore: fix circular dependency with xml and workspace comments

3556a3c

BeksOmega added 8 commits July 27, 2022 15:16

chore: fix stubbing parts of connection checker

58371ea

chore: fix stubbing dialog

59ce115

chore: fix stubbing style

8984901

chore: fix spying on duplicate

f3c7f17

chore: fix stubbing variables

47d8b49

chore: fix stubbing copy

50e4a84

chore: fix stubbing in workspace

8f6784a

chore: remove unnecessary stubs

a64ee7a

maribethb approved these changes Jul 28, 2022

View reviewed changes

core/blockly.ts Outdated Show resolved Hide resolved

core/clipboard.ts Outdated Show resolved Hide resolved

core/mutator.ts Outdated Show resolved Hide resolved

cpcallen requested changes Jul 28, 2022

View reviewed changes

core/dialog.ts Outdated Show resolved Hide resolved

core/dialog.ts Outdated Show resolved Hide resolved

core/utils/style.ts Outdated Show resolved Hide resolved

BeksOmega added 11 commits July 28, 2022 17:25

chore: fix formatting

36fb8a6

chore: fix other formatting

2394b23

chore: add backwards compatible static properties to workspace

f529d54

chore: move static type properties

c4ff05f

chore: move and comment stubs

72b9f29

chore: add newlines at EOF

fdd6cd5

chore: improve errors for monkey patched functions

b61be32

chore: update comment with a pointer to the doc

3630087

chore: update comment with a pointer to the doc

1beab2b

chore: format

5ee0bc5

chore: merge in ts/migration

4f6bd1c

BeksOmega merged commit 9d5dcc6 into google:ts/migration Jul 28, 2022

cpcallen reviewed Jul 29, 2022

View reviewed changes

BeksOmega deleted the ts/import-type-fix-circs-closure branch October 4, 2022 18:11

cpcallen mentioned this pull request Feb 23, 2023

Remove Closure library and module system from the Blockly codebase? #6858

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: circular dependencies #6281

fix: circular dependencies #6281

BeksOmega commented Jul 22, 2022 •

edited

Loading

cpcallen Jul 25, 2022

BeksOmega Jul 25, 2022

BeksOmega Jul 25, 2022

cpcallen Jul 25, 2022

cpcallen left a comment

cpcallen commented Jul 25, 2022

maribethb left a comment

cpcallen left a comment

cpcallen left a comment

cpcallen Jul 29, 2022

BeksOmega Jul 29, 2022

BeksOmega Jul 29, 2022

fix: circular dependencies #6281

fix: circular dependencies #6281

Conversation

BeksOmega commented Jul 22, 2022 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

Future work

cpcallen Jul 25, 2022

Choose a reason for hiding this comment

BeksOmega Jul 25, 2022

Choose a reason for hiding this comment

BeksOmega Jul 25, 2022

Choose a reason for hiding this comment

cpcallen Jul 25, 2022

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

cpcallen commented Jul 25, 2022

maribethb left a comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

cpcallen Jul 29, 2022

Choose a reason for hiding this comment

BeksOmega Jul 29, 2022

Choose a reason for hiding this comment

BeksOmega Jul 29, 2022

Choose a reason for hiding this comment

BeksOmega commented Jul 22, 2022 •

edited

Loading