fix: remove some circular dependencies #663
Conversation
Detected by rollup application in the maxgraph-integration-examples repository (!) Circular dependencies node_modules/@maxgraph/core/lib/util/Utils.js -> node_modules/@maxgraph/core/lib/util/config.js -> node_modules/@maxgraph/core/lib/util/logger.js -> node_modules/@maxgraph/core/lib/util/Utils.jsg node_modules/@maxgraph/core/lib/view/Graph.js -> node_modules/@maxgraph/core/lib/view/mixins/_graph-mixins-apply.js -> node_modules/@maxgraph/core/lib/view/Graph.js node_modules/@maxgraph/core/lib/editor/Editor.js -> node_modules/@maxgraph/core/lib/serialization/ModelXmlSerializer.js -> node_modules/@maxgraph/core/lib/serialization/register.js -> node_modules/@maxgraph/core/lib/serialization/codecs/index.js -> node_modules/@maxgraph/core/lib/serialization/codecs/editor/index.js -> node_modules/@maxgraph/core/lib/serialization/codecs/editor/EditorCodec.js -> node_modules/@maxgraph/core/lib/editor/Editor.js EXTRA move test to the right location
WalkthroughThis pull request introduces several changes across the repository. The CI pipeline is enhanced by adding a circular dependency check step to the GitHub Actions workflow and updating the corresponding npm script and dev dependency. Several import paths and module references have been updated in both test and source files. The serialization exports and codec registration are restructured with new modular files, and the utility function for calculating elapsed time has been relocated from one module to another. Additionally, the graph mixin application now explicitly accepts the Graph class as an argument. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant RM as registerModelCodecs
participant CR as CodecRegistry
C->>RM: Call registerModelCodecs(force)
RM->>RM: Check if codecs already registered
alt Not registered or force true
RM->>CR: Register CellCodec, ModelCodec, mxCellCodec, mxGeometryCodec
RM->>RM: Call registerBaseCodecs for shared codecs
RM->>CR: Set codec aliases (mxGraphModel, mxPoint)
RM->>RM: Mark codecs as registered
else Already registered
RM-->>C: Return without re-registration
end
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/serialization/register-shared.ts (1)
17-17: Fix grammatical error in the comment.The comment has a subject-verb agreement issue.
-// This function are private and are not intended to be exported by the npm package +// These functions are private and are not intended to be exported by the npm package
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.github/workflows/build.yml(1 hunks)packages/core/__tests__/view/GraphDataModel.test.ts(1 hunks)packages/core/package.json(2 hunks)packages/core/src/gui/MaxLog.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/serialization/ModelXmlSerializer.ts(1 hunks)packages/core/src/serialization/codecs/_model-codecs.ts(1 hunks)packages/core/src/serialization/codecs/_other-codecs.ts(0 hunks)packages/core/src/serialization/register-model-codecs.ts(1 hunks)packages/core/src/serialization/register-other-codecs.ts(1 hunks)packages/core/src/serialization/register-shared.ts(1 hunks)packages/core/src/util/Utils.ts(0 hunks)packages/core/src/util/logger.ts(1 hunks)packages/core/src/view/Graph.ts(1 hunks)packages/core/src/view/mixins/_graph-mixins-apply.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/core/src/util/Utils.ts
- packages/core/src/serialization/codecs/_other-codecs.ts
✅ Files skipped from review due to trivial changes (3)
- packages/core/tests/view/GraphDataModel.test.ts
- packages/core/src/serialization/codecs/_model-codecs.ts
- packages/core/src/serialization/ModelXmlSerializer.ts
🔇 Additional comments (17)
packages/core/src/view/mixins/_graph-mixins-apply.ts (2)
18-18: Good use of type-only import to break circular dependency.Converting the Graph import to a type-only import helps break the circular dependency while maintaining type safety.
43-44: Well-structured mixin application with explicit target.The function now accepts a target parameter instead of directly referencing Graph, which:
- Breaks the circular dependency
- Makes the function more flexible and reusable
- Makes the mixin application more explicit
packages/core/src/view/Graph.ts (1)
1490-1490: Appropriate placement of mixin application.The explicit passing of Graph to applyGraphMixins is well-placed after the full class definition, ensuring all properties and methods are available when mixins are applied.
packages/core/src/util/logger.ts (2)
19-27: LGTM! Well-documented utility function.The function is properly documented, includes type annotations, and is correctly marked as private to prevent external dependencies. This move helps break circular dependencies by placing the function in a more logically appropriate module.
77-82: LGTM! Clean integration with ConsoleLogger.The integration with ConsoleLogger.leave is clean and maintains the existing behavior while utilizing the newly relocated function.
packages/core/src/gui/MaxLog.ts (2)
25-26: LGTM! Clean import separation.The imports have been properly separated, helping to break circular dependencies while maintaining clean module boundaries.
268-273: LGTM! Consistent behavior maintained.The MaxLog.leave method continues to work as expected with the relocated getElapseMillisecondsMessage function.
packages/core/src/serialization/register-other-codecs.ts (3)
30-30: Great job consolidating imports under./codecs/_other-codecs.This change helps centralize codec definitions and reduces the likelihood of circular references, as each codec is now sourced from a single location.
37-37: Verify for potential circular references withregister-shared.Although splitting out
registerBaseCodecsis beneficial for modularity, please ensure thatregister-shareddoes not import anything here that might inadvertently reintroduce circular dependencies.If necessary, run a circular dependency checker (like madge) on the new file structure to confirm there are no loops.
38-38: Good separation of responsibilities by movingregisterModelCodecsto its own file.Extracting model-related codecs into
register-model-codecsimproves clarity and makes it easier to troubleshoot or expand that functionality without affecting unrelated codecs.packages/core/package.json (2)
50-50: LGTM! Script for checking circular dependencies.The script configuration looks good and aligns with the PR objectives. It correctly uses madge to check for circular dependencies in the JavaScript files within the lib directory.
62-62: Verify the madge package version.The specified version
~8.0.0seems incorrect as it's higher than the latest available version.✅ Verification successful
🌐 Web query:
What is the latest version of madge package on npm?💡 Result:
The latest version of the madge package on npm is 8.0.0, which was published 5 months ago according to npm registry listings[2][4][6]. This information comes directly from npm's official package pages and release history documentation[2][4].
Key details:
- Current version: 8.0.0
- First release: May 20, 2012 (version 0.0.1)[3]
- Maintained for over a decade with regular updates
- Most recent update before current: 5.0.2 in January 2023[7]
The package has seen continuous development with notable milestones including TypeScript support added in v3.6.0 (2019) and major architectural changes in v1.0.0 (2016)[1][7].
Citations:
- 1: https://unpkg.com/browse/madge@4.0.2/CHANGELOG.md
- 2: https://www.npmjs.com/package/madge/v/0.1.4?activeTab=versions
- 3: https://www.npmjs.com/package/madge/v/0.0.1
- 4: https://www.npmjs.com/package/madge/v/0.0.5?activeTab=versions
- 5: https://www.npmjs.com/package/madge/v/0.5.2?activeTab=versions
- 6: https://www.npmjs.com/package/@types/madge
- 7: https://github.com/pahen/madge/blob/master/CHANGELOG.md
- 8: https://secure.software/npm/packages/madge/5.0.2
- 9: https://www.npmjs.com/package/madge/v/3.4.0
- 10: https://security.snyk.io/package/npm/madge
Madge package version is correct. The current npm registry shows version 8.0.0 as the latest release, which matches the version specified in the
packages/core/package.jsonfile.
- The entry
"madge": "~8.0.0"correctly reflects the latest version available..github/workflows/build.yml (1)
75-76: LGTM! Step for checking circular dependencies.The step is correctly placed after all build steps to ensure the JS files are available for checking. The command properly targets the core package using the workspace flag.
packages/core/src/serialization/register-shared.ts (1)
22-30: LGTM! Well-structured base codec registration.The implementation effectively prevents duplicate registrations while allowing forced re-registration when needed. The use of a module-level flag is a clean way to maintain registration state.
packages/core/src/serialization/register-model-codecs.ts (2)
29-33: LGTM! Well-designed helper function.The
createObjectCodechelper function effectively encapsulates the common pattern of creating and naming object codecs, promoting code reuse.
44-63: LGTM! Well-structured model codec registration.The implementation is thorough and well-documented:
- Prevents duplicate registrations
- Handles backward compatibility with mxGraph
- Includes clear comments explaining the registration order
packages/core/src/index.ts (1)
98-101: LGTM! Improved modularity in exports.The changes effectively split the codec exports into more specific modules, which should help prevent circular dependencies while maintaining a clean public API.
|
1 participant
Introduce the usage of the
madgetool to detect such problems, especially because we already add a similar in the past fixed in df98075.The detection of the circular dependencies now runs as part of the CI build, so similar problems will be detected earlier than in the past.
Detected by rollup application in the maxgraph-integration-examples repository
(!) Circular dependencies
node_modules/@maxgraph/core/lib/util/Utils.js -> node_modules/@maxgraph/core/lib/util/config.js -> node_modules/@maxgraph/core/lib/util/logger.js -> node_modules/@maxgraph/core/lib/util/Utils.jsg
node_modules/@maxgraph/core/lib/view/Graph.js -> node_modules/@maxgraph/core/lib/view/mixins/_graph-mixins-apply.js -> node_modules/@maxgraph/core/lib/view/Graph.js
node_modules/@maxgraph/core/lib/editor/Editor.js -> node_modules/@maxgraph/core/lib/serialization/ModelXmlSerializer.js -> node_modules/@maxgraph/core/lib/serialization/register.js -> node_modules/@maxgraph/core/lib/serialization/codecs/index.js -> node_modules/@maxgraph/core/lib/serialization/codecs/editor/index.js -> node_modules/@maxgraph/core/lib/serialization/codecs/editor/EditorCodec.js -> node_modules/@maxgraph/core/lib/editor/Editor.js
Notes
Resources:
madge: https://github.com/pahen/madgeSummary by CodeRabbit
Chores
Refactor
Tests
New Features