-
Notifications
You must be signed in to change notification settings - Fork 390
fix: Add babel configuration for Playwright to handle TypeScript declare fields #5515
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❌ Some tests failed ⏰ Completed at: 09/25/2025, 10:37:33 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
2f74da7 to
0ae6e26
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
e1c0f11 to
9dd7ada
Compare
… and fix import path
- Created configurable babel-plugin-inject-globals to inject browser setup - Added setup-browser-globals.mjs for browser environment mocking - Moved babel plugin files to scripts directory for better organization - Removed dependency on import order by using babel transformation - Made plugin options configurable (filenamePattern, setupFile) - Updated tsconfig.json to include playwright config and scripts This fixes the ReferenceError: location is not defined issue that occurred when running pnpm collect-i18n, ensuring the command works reliably regardless of import auto-sorting. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Convert id to string before localeCompare to handle keyof Settings type - Ensure proper type casting for settings id field
c99e92e to
20e1cb0
Compare
This was automatically added by linter but causes issues with TypeScript compilation
Keep only the required babel dependencies: - @babel/plugin-transform-typescript - babel-plugin-module-resolver Revert all other unrelated version changes to minimize diff
Update lockfile after reverting unnecessary version changes
…g/ComfyUI_frontend into sno-fix-playwright-babel-config
This reverts commit 916170e.
…stubbing Vue imports to clean up the codebase
| update-locales: | ||
| # Branch detection: Only run for manual dispatch or version-bump-* branches from main repo | ||
| if: github.event_name == 'workflow_dispatch' || (github.event.pull_request.head.repo.full_name == github.repository && startsWith(github.head_ref, 'version-bump-')) | ||
| if: github.event_name == 'workflow_dispatch' || (github.event.pull_request.head.repo.full_name == github.repository && (startsWith(github.head_ref, 'version-bump-') || startsWith(github.head_ref, 'sno-fix-playwright-'))) |
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.
this is for testing, can be resotred after/before merge this pr
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.
probably before right?
|
Gonna close this PR in favor of this simplified solution: - fix(collect-i18n-node-defs): refactor to run ComfyNodeDefImpl only in browser context by snomiao · Pull Request #5775 · Comfy-Org/ComfyUI_frontend |
… browser context (#5775) ## The Problem The `collect-i18n-node-defs.ts` script started failing ~3 weeks ago when Vue nodes were introduced ([commit 006e6bd](006e6bd57), [PR #4263](#4263)). The issue stems from: 1. **Import chain bringing Vue components into Node.js context:** ``` collect-i18n-node-defs.ts ↓ imports ComfyNodeDefImpl (from nodeDefStore.ts) ↓ imports useSubgraphStore (from subgraphStore.ts) ↓ transitively imports executionStore.ts ↓ imports ChatHistoryWidget.vue (Vue component!) ``` 2. **TypeScript `declare` fields causing Babel errors:** ``` TypeScript 'declare' fields must first be transformed by @babel/plugin-transform-typescript ``` ## This Solution vs PR #5515 ### PR #5515 Approach (Complex) - Adds custom Babel plugins and configurations - Implements automatic browser globals injection - Requires **47,517 additions, 9,469 deletions** - Modifies the entire Playwright babel transformation pipeline ### This PR's Approach (Simple) - Uses dynamic imports to defer module loading until runtime - Avoids Babel compilation of problematic TypeScript/Vue files - **Only 40 lines changed** in a single file - No configuration changes needed ## How This Fix Works ```typescript // Instead of static import that Babel tries to compile: // import { ComfyNodeDefImpl } from '../src/stores/nodeDefStore' // We use: // 1. Type-only import (erased at runtime) import type { ComfyNodeDefImpl } from '../src/stores/nodeDefStore' // 2. Dynamic import at runtime (bypasses Babel) const { ComfyNodeDefImpl: ComfyNodeDefImplClass } = await import( '../src/stores/nodeDefStore' ) ``` --------- Co-authored-by: github-actions <[email protected]>
… browser context (#5775) ## The Problem The `collect-i18n-node-defs.ts` script started failing ~3 weeks ago when Vue nodes were introduced ([commit 006e6bd](006e6bd57), [PR #4263](#4263)). The issue stems from: 1. **Import chain bringing Vue components into Node.js context:** ``` collect-i18n-node-defs.ts ↓ imports ComfyNodeDefImpl (from nodeDefStore.ts) ↓ imports useSubgraphStore (from subgraphStore.ts) ↓ transitively imports executionStore.ts ↓ imports ChatHistoryWidget.vue (Vue component!) ``` 2. **TypeScript `declare` fields causing Babel errors:** ``` TypeScript 'declare' fields must first be transformed by @babel/plugin-transform-typescript ``` ## This Solution vs PR #5515 ### PR #5515 Approach (Complex) - Adds custom Babel plugins and configurations - Implements automatic browser globals injection - Requires **47,517 additions, 9,469 deletions** - Modifies the entire Playwright babel transformation pipeline ### This PR's Approach (Simple) - Uses dynamic imports to defer module loading until runtime - Avoids Babel compilation of problematic TypeScript/Vue files - **Only 40 lines changed** in a single file - No configuration changes needed ## How This Fix Works ```typescript // Instead of static import that Babel tries to compile: // import { ComfyNodeDefImpl } from '../src/stores/nodeDefStore' // We use: // 1. Type-only import (erased at runtime) import type { ComfyNodeDefImpl } from '../src/stores/nodeDefStore' // 2. Dynamic import at runtime (bypasses Babel) const { ComfyNodeDefImpl: ComfyNodeDefImplClass } = await import( '../src/stores/nodeDefStore' ) ``` --------- Co-authored-by: github-actions <[email protected]>
Summary
Fixes babel configuration issues in the Playwright i18n collection script that were causing failures when importing
ComfyNodeDefImplafter Vue nodes were introduced to the codebase.The Problem
The
collect-i18n-node-defs.tsscript was failing because of an import chain that brings Vue components into Node.js context:The Import Chain
Critical Issue: When babel tries to transform
.vuefiles in Node.js context, it crashes because Vue SFC files are not JavaScript - they're templates that need webpack/rollup loaders, not babel transpilation.When This Started
This problem began ~3 weeks ago when Vue nodes were introduced in commit 006e6bd57 (PR #4263). Before Vue nodes existed, the import chain didn't include any
.vuefiles, so the script worked fine.The Solution
This PR implements a robust babel transformation pipeline that:
1. Enhanced Playwright Configuration
config['@playwright/test']configuration to control babel transformations2. Automatic Browser Globals Injection
babel-plugin-inject-globals.cjsthat automatically injects browser environment setup for matching files3. Browser Context Evaluation
collect-i18n-node-defs.tsto evaluate imports within browser context using Playwright4. Cleanup
babel-plugin-stub-vue-imports.cjs(no longer needed with proper babel configuration)The Learning Journey
Three weeks ago, we decided to pause running 'update locales' on every PR and moved this process to the version-bump-* branches. Later, update-locales started failing.
Initial Attempts That Failed:
First attempt: I thought it was simply due to the new
declaresyntax in litegraph, so I tried removingdeclarekeywords (PR #5304). However, this immediately caused semantic changes after compiling to JS, causing vitest to fail in unexpected ways.Second attempt: I tried using regex to strip
declarekeywords before and after playwright.i18n startup (PR #5297). This worked initially and generated some 'Update locales [skip ci]' commits, but as we merged from origin/main (our team develops very fast!), completely different errors kept appearing, leaving me clueless.Type Import Investigation: Initially, I suspected the issue was related to type imports being incorrectly included at runtime, so I started working on
verbatimModuleSyntax. During this process, I encountered extremely complex circular dependency issues (see PR #5533 comment for detailed analysis).Circular Dependency Discovery
To investigate circular dependencies in the project, I created a simple import-map visualization tool using D3.js that highlights all circular paths and uses physics models for automatic clustering: snomiao/ComfyUI_frontend-import-map-analyze
This tool helped identify critical circular dependencies like:
EmptySubgraphInput → SubgraphInput → SubgraphInputNode → EmptySubgraphInputSubgraphNode → LGraphNode → litegraph.ts → SubgraphNodeSpecial thanks to @DrJKL and @webfiltered for their help in completely resolving the type import (verbatimModuleSyntax) issues.
The Breakthrough:
After extensive research on how Playwright, Babel, and Vue work, and with Christian Byrne's suggestion to study Playwright's source code, I discovered the magic
config['@playwright/test']configuration option. This allowed me to finally understand how Playwright handles TypeScript and Vue.The Key Realization:
After much research into Vue/JSX/Webpack/Rollup, I realized a fundamental difference:
.vuefiles are NOT JavaScript that can be compiled by Babel. They are templates (more like.htmlfiles) that should be loaded by webpack/rollup, not transpiled by BabelThe critical insight: We should NEVER import any
.vuefiles in Playwright's Node environment. Instead, they should run entirely in the browser and usepage.evaluate()to pass JSON data to the Node environment.Changes
babel-plugin-inject-globals.cjsfor automatic browser globals injectionplaywright.i18n.config.tswith complete babel transformation pipelinecollect-i18n-node-defs.tsto use browser context evaluationbabel-plugin-stub-vue-imports.cjsTest Plan
Why This Fix Works
Previously, when
ComfyNodeDefImplimported Vue components through the chain shown above, babel would try to transform them in Node.js context and fail. Now:Related Issues
Fixes issues with
pnpm collect-i18nfailing after Vue nodes were introduced to the codebase.🤖 Generated with Claude Code
┆Issue is synchronized with this Notion page by Unito