refactor: Remove last remaining circular import in core/#6818
Merged
cpcallen merged 7 commits intoFeb 7, 2023
Conversation
Collaborator
Author
|
Leaving this as a draft for the moment because it turns out I missed some calls to |
cpcallen
force-pushed
the
fix/6817-no-cyclic-imports
branch
2 times, most recently
from
dbd8a20 to
d0783af
Compare
This function being in core/xml.ts was the cause for the last remaining circular import in core/ (between variables.ts and xml.ts). Moving it to utils/xml.ts makes sense anyway, since there is nothing Blockly-specific about this function. Fixes RaspberryPiFoundation#6817.
Reenable an assertion which check to make sure that goog.declareModuleId is not called more than once in a module (and which also catches circular imports amongst ES modules, which are not detected by closure-make-deps).
cpcallen
force-pushed
the
fix/6817-no-cyclic-imports
branch
from
d0783af to
f409de7
Compare
Testing the migration file entry by auto-migrating all uses of Blockly.Xml.textToDom to Blockly.utils.xml.textToDom.
Update the one remaining call to textToDom (in blocks/lists.ts) to the function's new location - also removing the last use of the Blockly.Xml / core/xml.ts) module from this file.
rachel-fenichel
approved these changes
Feb 6, 2023
Collaborator
rachel-fenichel
left a comment
rachel-fenichel
left a comment
There was a problem hiding this comment.
LGTM once you fix conflicts.
cpcallen
added a commit
to cpcallen/blockly
that referenced
this pull request
Feb 8, 2023
Prior to PR RaspberryPiFoundation#6818, circular imports resulted in the debug module loader (in closure/goog/base.js) failing to record the goog.module ID of most modules that were involved in the cycle, and in particular of the Blockly.Xml module. This had secondary fallout that resulted in library blocks modules being loaded in the wrong order. A kludge was introduced in PR RaspberryPiFoundation#6703 that worked around this problem by making sure that window.Blockly was set, allowing the modules loaded out-of-order to still work. Now that we have removed all remaining circular dependencies there is no need for the kludge, since all module IDs are properly recorded and modules are loaded in the correct order.
4 tasks
cpcallen
added a commit
that referenced
this pull request
Feb 8, 2023
* chore(tests): Remove circular import loading issue kludge Prior to PR #6818, circular imports resulted in the debug module loader (in closure/goog/base.js) failing to record the goog.module ID of most modules that were involved in the cycle, and in particular of the Blockly.Xml module. This had secondary fallout that resulted in library blocks modules being loaded in the wrong order. A kludge was introduced in PR #6703 that worked around this problem by making sure that window.Blockly was set, allowing the modules loaded out-of-order to still work. Now that we have removed all remaining circular dependencies there is no need for the kludge, since all module IDs are properly recorded and modules are loaded in the correct order. * chore(build): Remove exclude for non-existent core/blockly.js There was a transitional period where we had both core/blockly.ts and core/blockly.js, and wished to exclude the latter from tsc's input, but the latter file was deleted (and inadvertently restored, then re-deleted) some time ago.
20 tasks
oszizsolt
pushed a commit
to oszizsolt/react-blockly
that referenced
this pull request
Jul 28, 2023
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #6817.
Proposed Changes
textToDomfromcore/xml.tstocore/utils/xml.ts.goog.declareModuleIdmultiple-call check inclosure/goog/base.js.Reason for Changes
The
textToDomfunction being incore/xml.tsis the cause for the last remaining circularimportincore/(betweenvariables.tsandxml.ts). Moving it tocore/utils/xml.tsmakes sense anyway, since there is nothing Blockly-specific about this function.Reenabling the assertion which checks to make sure that
goog.declareModuleIdis not called more than once in a module will also catch circularimports amongst ES modules, which are not detected byclosure-make-deps).Test Coverage
Passes
npm test.No manual test changes expected.