refactor: Remove TypeScript declare keywords for Playwright compatibility #5304

snomiao · 2025-09-02T14:45:16Z

Summary

Replaced TypeScript declare keywords with standard TypeScript patterns
Removed babel preprocessing dependencies and setup files
Simplified i18n collection test configuration

Problem

The current implementation uses TypeScript's declare keyword for class property declarations, which causes issues with Playwright's node environment that doesn't have proper TypeScript support. This required a complex babel preprocessing step during i18n collection tests.

Solution

Instead of preprocessing files with babel to remove declare keywords, this PR replaces them with standard TypeScript patterns:

For type narrowing in subclasses: Use getter/setter with type assertions
For optional properties: Remove declare and use plain property declarations

Changes

Removed babel dependencies (@babel/core, @babel/preset-typescript) from package.json
Deleted i18n setup files (globalSetupWithI18n.ts, globalTeardownWithI18n.ts, i18nSetup.ts)
Updated playwright.i18n.config.ts to use regular setup instead of custom preprocessing
Modified 7 TypeScript files to replace declare keywords with standard patterns

Test plan

Run pnpm typecheck to verify TypeScript compilation
Run pnpm lint to check for linting issues
Run pnpm collect-i18n to verify i18n collection works without babel preprocessing
Run pnpm test:unit to ensure no regressions in unit tests

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

github-actions · 2025-09-02T14:45:30Z

🎭 Playwright Test Results

❌ Some tests failed

⏰ Completed at: 09/22/2025, 11:11:49 PM UTC

📈 Summary

Total Tests: 457
Passed: 425 ✅
Failed: 2 ❌
Flaky: 1 ⚠️
Skipped: 29 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 418 / ❌ 2 / ⚠️ 1 / ⏭️ 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.

github-actions · 2025-09-02T16:25:39Z

🔧 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

christian-byrne

We should investigate if this can be done without type assertions.

christian-byrne · 2025-09-04T06:15:12Z

src/lib/litegraph/src/subgraph/SubgraphOutput.ts

    afterRerouteId?: RerouteId
  ): LLink | undefined {
-    const { subgraph } = this.parent
+    const parent = this.parent as SubgraphOutputNode


Can this be done without assertions?

christian-byrne · 2025-09-04T06:16:00Z

src/lib/litegraph/src/subgraph/SubgraphNode.ts

 export class SubgraphNode extends LGraphNode implements BaseLGraph {
-  declare inputs: (INodeInputSlot & Partial<ISubgraphInput>)[]
+  // Override inputs with proper typing for subgraph inputs
+  override inputs: (INodeInputSlot & Partial<ISubgraphInput>)[] = []


Switching to override is necessary?

This class extends LGraphNode which have inputs def shaped like this: {inputs: INodeInputSlot[] = []}

I think it's necessary to use override or you will have find another way to solve this error:

"This member must have an 'override' modifier because it overrides a member in the base class 'LGraphNode'.ts(4114)"

This class extends LGraphNode which have inputs def shaped like this: {inputs: INodeInputSlot[] = []}

I think it's necessary to use override or you will have to find another way to solve this error:

This member must have an 'override' modifier because it overrides a member in the base class 'LGraphNode'.ts(4114)

christian-byrne · 2025-09-04T06:16:08Z

src/lib/litegraph/src/subgraph/SubgraphInput.ts

    afterRerouteId?: RerouteId
  ): LLink | undefined {
-    const { subgraph } = this.parent
+    const parent = this.parent as SubgraphInputNode


Can this be done without assertions?

christian-byrne · 2025-09-04T06:16:12Z

src/lib/litegraph/src/subgraph/EmptySubgraphOutput.ts

    afterRerouteId?: RerouteId
  ): LLink | undefined {
-    const { subgraph } = this.parent
+    const { subgraph } = this.parent as SubgraphOutputNode


Can this be done without assertions?

christian-byrne · 2025-09-04T06:16:15Z

src/lib/litegraph/src/subgraph/EmptySubgraphInput.ts

    afterRerouteId?: RerouteId
  ): LLink | undefined {
-    const { subgraph } = this.parent
+    const { subgraph } = this.parent as SubgraphInputNode


Can this be done without assertions?

christian-byrne · 2025-09-04T06:16:19Z

src/lib/litegraph/src/canvas/FloatingRenderLink.ts

    floatingLink.origin_id = SUBGRAPH_OUTPUT_ID
-    floatingLink.origin_slot = output.parent.slots.indexOf(output)
+    floatingLink.origin_slot = (
+      output.parent as SubgraphOutputNode


Can this be done without assertions?

christian-byrne · 2025-09-04T06:16:22Z

src/lib/litegraph/src/canvas/FloatingRenderLink.ts

    floatingLink.origin_id = SUBGRAPH_INPUT_ID
-    floatingLink.origin_slot = input.parent.slots.indexOf(input)
+    floatingLink.origin_slot = (
+      input.parent as SubgraphInputNode


Can this be done without assertions?

snomiao · 2025-09-04T21:19:27Z

TL;DR

My claude generated a util file added a bunch of "A instanceof B" helper functions to narrowing the types,

Overall SGTM, its not failing any tests yet but I am a bit concern about backward compabilities on custom nodes.

also cc @webfiltered

--- below claude message ---

✅ Review feedback addressed

@christian-byrne I've addressed your review comment about investigating the removal of type assertions. Here's what I changed:

Changes made:

Added type guards to :
- isSubgraphInputNode() - Runtime type guard for SubgraphInputNode
- isSubgraphOutputNode() - Runtime type guard for SubgraphOutputNode
Replaced all type assertions with proper type guard checks:
- SubgraphInput.ts - Now uses isSubgraphInputNode() instead of as SubgraphInputNode
- SubgraphOutput.ts - Now uses isSubgraphOutputNode() instead of as SubgraphOutputNode
- EmptySubgraphInput.ts - Updated to use type guard
- EmptySubgraphOutput.ts - Updated to use type guard
- FloatingRenderLink.ts - Both connect methods now use type guards
Added error handling - Each type guard check includes error logging if the type check fails

Benefits:

✅ Type safety - Runtime validation instead of unsafe assertions
✅ Better error detection - Will log errors if parent types are incorrect
✅ Cleaner code - Explicit type checking makes the code more maintainable

The solution provides proper runtime type validation while maintaining TypeScript's type safety guarantees.

snomiao · 2025-09-04T22:22:00Z

Update: Reverted type guard changes

After investigating the failing tests, I've reverted the type guard changes. The issue was that adding runtime type guards with early returns could potentially break execution flow if the parent types aren't initialized exactly as expected during certain edge cases.

Current state of PR:

✅ Original goal achieved - Removed TypeScript declare keywords from class properties
✅ Properties properly initialized - All properties now have explicit initializers
❌ Type guards reverted - The type assertion approach, while not ideal, doesn't break functionality

Why the revert:

The type guards added defensive error checking that would return early if parent types didn't match expectations. This could interrupt normal execution flow during edge cases that the tests were catching. The original type assertions, while less type-safe, allow the code to continue executing.

The PR now contains only the original changes to remove declare keywords, which was the main objective.

socket-security · 2025-09-04T22:22:05Z

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

snomiao · 2025-09-05T00:27:38Z

@christian-byrne

Can this be done without assertions?

Prob not, I have tested so far:

Add forced type guards (aka instanceof ) breaks our playwright tests.
Ignore the type assertion causes TS typecheck error

cc @webfiltered @snomiao

snomiao · 2025-09-05T00:29:57Z

Test Failure Analysis

The type guard implementation caused 2 Playwright tests to fail:

Failed Tests:

execution.spec.ts:6:3 - "Report error on unconnected slot"
execution.spec.ts:23:3 - "Execute to selected output nodes"

Root Cause:

The type guard implementation added defensive checks like this:

if (!isSubgraphInputNode(this.parent)) {
  console.error('Invalid parent type for SubgraphInput')
  return  // ← Early return breaks execution
}

Why It Failed:

The early return statements interrupted the normal execution flow
During certain runtime scenarios in the tests, the parent object might not strictly match the instanceof check used in the type guards
This could happen due to:
- Object prototype chains being different than expected
- Objects being created through serialization/deserialization
- Dynamic object construction during test setup

The Fix:

Reverted to the original type assertions (as SubgraphInputNode) which:

Don't interrupt execution flow
Allow the code to proceed even if types aren't perfectly matched at runtime
While less type-safe, they maintain the existing working behavior

The tests now pass because the execution flow continues uninterrupted, as it did before the type guard changes.

christian-byrne · 2025-09-06T19:38:57Z

We need to just look into the type issue manually, Claude is really bad at solving these things because it also requires some semantic analysis and advanced types knowledge.

- Remove declare keywords from LGraphNode properties - Remove declare keywords from SubgraphInput/Output parent properties - Remove declare keywords from BaseWidget properties - Add proper type assertions for parent property access - Fix async route handling in collect-i18n-node-defs.ts This resolves TypeScript compilation issues and improves type safety.

snomiao · 2025-09-26T06:03:53Z

close this as not necessary anymore because we solved locale workflow problem here: - fix(collect-i18n-node-defs): refactor to run ComfyNodeDefImpl only in browser context by snomiao · Pull Request #5775 · Comfy-Org/ComfyUI_frontend

christian-byrne requested changes Sep 4, 2025

View reviewed changes

This was referenced Sep 5, 2025

refactor: Improve litegraph preprocessing for i18n collection #5297

Closed

[fix] Remove useDefineForClassFields from tsconfig #5332

Closed

snomiao force-pushed the sno-fix-playwright-remove-declare branch from 6af4b2b to 08275af Compare September 22, 2025 23:00

snomiao mentioned this pull request Sep 24, 2025

fix: Add babel configuration for Playwright to handle TypeScript declare fields #5515

Closed

snomiao closed this Sep 26, 2025

refactor: Remove TypeScript declare keywords for Playwright compatibility #5304

refactor: Remove TypeScript declare keywords for Playwright compatibility #5304

Uh oh!

Conversation

snomiao commented Sep 2, 2025 • edited by sync-by-unito bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Changes

Test plan

Uh oh!

github-actions bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎭 Playwright Test Results

📈 Summary

📊 Test Reports by Browser

Uh oh!

github-actions bot commented Sep 2, 2025

🔧 Auto-fixes Applied

Changes made:

Uh oh!

christian-byrne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snomiao commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Review feedback addressed

Changes made:

Benefits:

Uh oh!

snomiao commented Sep 4, 2025

Update: Reverted type guard changes

Current state of PR:

Why the revert:

Uh oh!

socket-security bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snomiao commented Sep 5, 2025

Uh oh!

snomiao commented Sep 5, 2025

Test Failure Analysis

Failed Tests:

Root Cause:

Why It Failed:

The Fix:

Uh oh!

christian-byrne commented Sep 6, 2025

Uh oh!

snomiao commented Sep 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snomiao commented Sep 2, 2025 •

edited by sync-by-unito bot

Loading

github-actions bot commented Sep 2, 2025 •

edited

Loading

snomiao commented Sep 4, 2025 •

edited

Loading

socket-security bot commented Sep 4, 2025 •

edited

Loading