Skip to content

Conversation

snomiao
Copy link
Member

@snomiao snomiao commented Sep 13, 2025

Summary

  • Enable verbatimModuleSyntax compiler option in TypeScript configuration
  • Update all type imports to use explicit import type syntax
  • This change will Improve tree-shaking and bundler compatibility

Motivation

The verbatimModuleSyntax option ensures that type-only imports are explicitly marked with the type keyword. This:

  • Makes import/export intentions clearer
  • Improves tree-shaking by helping bundlers identify what can be safely removed
  • Ensures better compatibility with modern bundlers
  • Follows TypeScript best practices for module syntax

Changes

  • Added "verbatimModuleSyntax": true to tsconfig.json
  • Updated another 48+ files to use explicit import type syntax for type-only imports
  • No functional changes, only import/export syntax improvements

Test Plan

  • TypeScript compilation passes
  • Build completes successfully
  • Tests pass
  • No runtime behavior changes

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

Copy link

github-actions bot commented Sep 13, 2025

🎭 Playwright Test Results

All tests passed!

⏰ Completed at: 09/19/2025, 03:45:01 AM UTC

📈 Summary

  • Total Tests: 450
  • Passed: 421 ✅
  • Failed: 0
  • Flaky: 0
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 414 / ❌ 0 / ⚠️ 0 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

DrJKL
DrJKL previously approved these changes Sep 14, 2025
Copy link
Contributor

@DrJKL DrJKL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it so much, thank you!

There are some lint rules that I think we should pair with this, but I'd rather see them as a follow-up to this PR so that they stand alone in the history.

@snomiao
Copy link
Member Author

snomiao commented Sep 14, 2025

Circular Dependency Issue with verbatimModuleSyntax

We've encountered a blocking issue when enabling TypeScript's verbatimModuleSyntax option. The strict import/export requirements expose circular dependencies that were previously hidden:

The Problem

With verbatimModuleSyntax enabled:

  1. Type imports must use import type { ... } syntax
  2. Values and types cannot be mixed in the same import statement
  3. This stricter separation reveals circular dependencies between modules

Actual Runtime Failures

When running tests with Vitest, we encounter multiple runtime errors that demonstrate circular dependency problems:

Test Failure 1: EmptySubgraphInput

FAIL  tests-ui/tests/store/bottomPanelStore.test.ts
TypeError: Class extends value undefined is not a constructor or null
 ❯ src/lib/litegraph/src/subgraph/EmptySubgraphInput.ts:14:41
     12|  * A virtual slot that simply creates a new input slot when connected …
     13|  */
     14| export class EmptySubgraphInput extends SubgraphInput {
       |                                         ^
     15|   declare parent: SubgraphInputNode
     16| 
 ❯ src/lib/litegraph/src/canvas/LinkConnector.ts:4:31

Test Failure 2: SubgraphNode

FAIL  tests-ui/tests/composables/useMissingNodes.test.ts
TypeError: Class extends value undefined is not a constructor or null
 ❯ src/lib/litegraph/src/subgraph/SubgraphNode.ts:37:35
     35|  * An instance of a {@link Subgraph}, displayed as a node on the conta…
     36|  */
     37| export class SubgraphNode extends LGraphNode implements BaseLGraph {
       |                                   ^
     38|   declare inputs: (INodeInputSlot & Partial<ISubgraphInput>)[]
     39| 
 ❯ src/lib/litegraph/src/litegraph.ts:67:32

These errors occur because parent classes are undefined at the time child classes try to extend them, indicating circular dependencies in the module initialization order.

Circular Import Chain 1: EmptySubgraphInput

The Cycle:

EmptySubgraphInput → SubgraphInput → SubgraphInputNode → EmptySubgraphInput
                                    ↓                    ↑
                                LinkConnector ──────────┘

Detailed chain:

  1. EmptySubgraphInput.ts

    • Line 8: import { SubgraphInput } from './SubgraphInput'
    • Line 14: export class EmptySubgraphInput extends SubgraphInput
  2. SubgraphInput.ts

    • Line 16: import type { SubgraphInputNode } from './SubgraphInputNode'
    • Line 18: import { SubgraphSlot } from './SubgraphSlotBase'
    • Line 32: export class SubgraphInput extends SubgraphSlot
  3. SubgraphInputNode.ts

    • Line 5: import type { LinkConnector } from '@/lib/litegraph/src/canvas/LinkConnector'
    • Line 19: import { EmptySubgraphInput } from './EmptySubgraphInput'
    • Line 30: Creates EmptySubgraphInput instance
  4. LinkConnector.ts

    • Line 20: import { EmptySubgraphInput } from '@/lib/litegraph/src/subgraph/EmptySubgraphInput'
    • Line 24: import { SubgraphInputNode } from '@/lib/litegraph/src/subgraph/SubgraphInputNode'

Circular Import Chain 2: SubgraphNode

The Cycle:

SubgraphNode → LGraphNode → litegraph.ts → SubgraphNode

Detailed chain:

  1. SubgraphNode.ts

    • Line 4: import { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
    • Line 37: export class SubgraphNode extends LGraphNode
  2. LGraphNode.ts

    • Lines 46-50: Imports from './litegraph' including Subgraph and SubgraphNode types
  3. litegraph.ts

    • Line 11: export { Subgraph } from './subgraph/Subgraph'
    • Line 67: References SubgraphNode (indirectly through imports)

When modules are loaded:

  1. litegraph.ts tries to export Subgraph
  2. This triggers loading of SubgraphNode (as it's part of the subgraph module)
  3. SubgraphNode tries to extend LGraphNode
  4. LGraphNode imports from litegraph.ts
  5. But litegraph.ts is still being initialized, causing LGraphNode to be undefined

Additional Circular Dependencies Found

  1. LiteGraph Internal Structure

    • Subgraph class extends LGraph (in LGraph.ts)
    • Multiple files import both Subgraph and components that depend on it
    • Temporary workaround exists: Subgraph.ts re-exports from LGraph.ts with comment "temporary fix to resolve circular dependency issues"
  2. ComfyApp ↔ LiteGraph

    • src/scripts/app.ts exports ComfyApp class
    • Various files import from both @comfyorg/litegraph and ComfyApp
    • LiteGraph extensions depend on ComfyApp types
    • ComfyApp depends on LiteGraph types
  3. Component Registry Circular References

    • Component index files export types
    • Components import from their own index for type definitions
    • Creates self-referential loops

Why This Matters

  • Build failures: TypeScript cannot resolve the import order
  • Runtime crashes: Even if TypeScript compiles, the circular dependencies cause runtime initialization failures
  • Test failures: Vitest cannot properly initialize modules with circular dependencies
  • Maintenance burden: The codebase architecture needs refactoring to properly separate concerns

Recommended Solution

Before enabling verbatimModuleSyntax, we should:

  1. Break the SubgraphInput cycle:

    • Option A: Extract EmptySubgraphInput to not extend SubgraphInput directly
    • Option B: Use lazy initialization for the emptySlot in SubgraphInputNode
    • Option C: Restructure the inheritance hierarchy to avoid the cycle
  2. Break the SubgraphNode cycle:

    • Option A: Move type imports to separate type-only modules
    • Option B: Use barrel exports consistently (as noted in LiteGraph's CLAUDE.md)
    • Option C: Restructure so LGraphNode doesn't need to import from litegraph.ts
  3. Fix LiteGraph internal structure:

    • Resolve circular dependencies within the LiteGraph library
    • Ensure proper initialization order for class hierarchies
    • Separate type definitions from implementations
  4. Refactor module structure to eliminate circular dependencies:

    • Extract shared types to separate type-only modules
    • Use dependency injection patterns where appropriate
    • Consider facade patterns for complex module interactions
  5. Create abstraction layers:

    • Define interfaces in separate files from implementations
    • Use type-only exports for shared contracts
    • Implement proper layering (core → features → UI)
  6. Gradual migration:

    • Fix circular dependencies module by module
    • Enable verbatimModuleSyntax per module once clean
    • Eventually enable project-wide

This refactoring would improve the codebase architecture beyond just TypeScript compliance - it would make the code more maintainable and testable.

Current Status

The branch has partial fixes for type imports, but the circular dependency issues block full implementation. The runtime errors in tests confirm this is not just a TypeScript compilation issue but a fundamental module structure problem that needs architectural refactoring.

Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@snomiao snomiao force-pushed the sno-verbatim-tsconfig branch from 2169769 to 8b81113 Compare September 17, 2025 16:55
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@snomiao snomiao force-pushed the sno-verbatim-tsconfig branch from 42185c6 to 8b24d6f Compare September 17, 2025 17:10
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@snomiao snomiao force-pushed the sno-verbatim-tsconfig branch from bcfb53e to 38ab14c Compare September 18, 2025 11:34
@snomiao snomiao marked this pull request as ready for review September 18, 2025 12:12
@snomiao snomiao requested review from jtydhr88 and a team as code owners September 18, 2025 12:12
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 18, 2025
@snomiao
Copy link
Member Author

snomiao commented Sep 18, 2025

huge thanks to @DrJKL , this branch finally works now

cc @christian-byrne

@snomiao snomiao requested a review from DrJKL September 18, 2025 12:13
@christian-byrne
Copy link
Contributor

Can you fix conflicts @snomiao ? TY

@snomiao snomiao force-pushed the sno-verbatim-tsconfig branch from 38ab14c to 09f79ec Compare September 19, 2025 03:34
@christian-byrne christian-byrne merged commit cbb0f76 into main Sep 19, 2025
21 checks passed
@christian-byrne christian-byrne deleted the sno-verbatim-tsconfig branch September 19, 2025 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants