-
Notifications
You must be signed in to change notification settings - Fork 448
[refactor] Solve circular dependency in LiteGraph module #5604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎭 Playwright Test Results⏰ Completed at: 09/24/2025, 03:00:45 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
55fb7ce to
a6480d1
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
c1ee727 to
eef0560
Compare
webfiltered
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misreading the PR - the current global LiteGraph object is already a singleton. Could you not just move that instead? It would be a significantly smaller change.
eef0560 to
8bb99a3
Compare
Hey @webfiltered, You did not misreading! I 've renamed it just wanna ensure we are importing direct from the module insteadof from barrel file litegraph.ts. Using a different name helps avoid any confusion between two export files in future. I think better name is LiteGraphInternal now, just pushed new commit for it : D |
aa4083f to
daa76ef
Compare
I'm still a bit confused - the imports should always work so long as you don't mess with the barrel file and import from it first. I balanced this a long while back, and messing with it is complex and frustrating to debug (especially with the ecosystem using the LiteGraph global var). I just ran PTAL: https://github.com/Comfy-Org/ComfyUI_frontend/compare/verbatim-module-syntax?expand=1 |
|
Argh ran the wrong tests. Okay, I see the error you saw. This is actually just an issue with the test files. |
|
Yeah, can fix this easily by doing type imports properly: import { type ThingToImport } from '@/file'This still imports the file when running tests. We don't notice it in prod because it uses different systems (vite/bundle). Should be: import type { ThingToImport } from '@/file'This type of import is only used for type information, pre-compile - doesn't exist at runtime. |
I think we can merge your solution as first step:
and then gradually refactor in following PRs into:
to make things work smoothy, WDYT? @webfiltered @DrJKL @christian-byrne |
|
I think if we're going to do this change and touch all the imports, we may as well just do it all at once. I would not want to keep having to sanity check this repeatedly. Pretty sure there's a linter that can auto-fix the imports. Failing that, we can just stuff the If there are more issues in future, it's just from direct importing. Runtime imports should be via the barrel, not direct importing from internal LiteGraph files. |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
95e0f09 to
8468b4e
Compare
8468b4e to
4a2007f
Compare
…ular dependencies
4a2007f to
d866ef7
Compare
|
Closed this as may not needed anymore since i18n locales workflow fixed by another approach #5775 |
Summary
Context
This PR addresses the second circular dependency issue mentioned in #5533:
SubgraphNode → LGraphNode → litegraph.ts → SubgraphNodelitegraph.tsexports modules, it triggers loading ofSubgraphNodewhich tries to extendLGraphNode, butLGraphNodeimports fromlitegraph.tscausing circular initializationSolution
By extracting the LiteGraph singleton into a separate
LiteGraphSingleton.tsmodule, we break the circular dependency chain and ensure proper module initialization order.Changes
LiteGraphSingleton.tsmodule to hold the singleton instanceTest Plan
🤖 Generated with Claude Code