refactor: migrate ES private fields to TypeScript private for Vue Proxy compatibility #8440
Conversation
- Rectangle.ts: #pos, #size - ConstrainedSize.ts: #width, #height, #desiredWidth, #desiredHeight - DragAndScale.ts: #stateHasChanged Amp-Thread-ID: https://ampcode.com/threads/T-019c0b60-f152-749f-9e38-4a67034ea58e Co-authored-by: Amp <amp@ampcode.com>
NodeSlot: #node → protected _node, #centreOffset → private _centreOffset NodeOutputSlot: removed redundant #node (uses inherited _node) NodeInputSlot: #widget → private _widgetRef BaseWidget: #node → private _node, #value → private _value Amp-Thread-ID: https://ampcode.com/threads/T-019c0b63-f429-755f-88db-0092230d4158 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0b67-3db9-734c-9356-49ec7626c73c Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0b6a-93a9-75db-be00-00d70b66628a Co-authored-by: Amp <amp@ampcode.com>
Convert ECMAScript private fields (#) to TypeScript private (_) in: - src/lib/litegraph/src/canvas/LinkConnector.ts - src/lib/litegraph/src/LGraphCanvas.ts Amp-Thread-ID: https://ampcode.com/threads/T-019c0b6f-2e60-76df-8acf-9aca8f1f6bff Co-authored-by: Amp <amp@ampcode.com>
LGraphNode.ts: #concreteInputs, #concreteOutputs, #renderArea, #boundingRect, #getErrorStrokeStyle, #getSelectedStrokeStyle, #findFreeSlot, #findSlotByType, #defaultVerticalInputs, #defaultVerticalOutputs, #getSlotPositionContext, #measureSlot, #measureSlots, #getMouseOverSlot, #isMouseOverSlot, #isMouseOverWidget, #arrangeWidgets, #arrangeWidgetInputSlots Amp-Thread-ID: https://ampcode.com/threads/T-019c0b7a-1a5c-72af-bcc8-8070d47afd82 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0b80-c4fc-7289-9ee3-94ec616d1108 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0b87-33a9-764a-a46d-9841bee4894c Co-authored-by: Amp <amp@ampcode.com>
- widgetInputs.ts: #onFirstConnection, #createWidget, #mergeWidgetConfig, #isValidConnection, #removeWidgets - groupNode.ts: #convertedToProcess Amp-Thread-ID: https://ampcode.com/threads/T-019c0b8a-b7c8-727c-ab6b-b178fbd3d5c7 Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c0b8d-c32f-7194-baee-ea37d5c13995
Add toJSON() methods to LGraph, LGraphCanvas, NodeSlot, NodeInputSlot, and NodeOutputSlot to break circular reference chains when class instances are accidentally passed to JSON.stringify(). Also make HTMLCanvasElement.data non-enumerable to prevent the LGraphCanvas back-reference from being traversed during serialization. These fixes address circular reference errors that appeared after migrating ES private fields (#) to TypeScript private (_), since TypeScript private fields are visible to JSON.stringify unlike true ES private fields. Amp-Thread-ID: https://ampcode.com/threads/T-019c0bba-2a2e-7281-997b-5fc25957c932 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0bba-2a2e-7281-997b-5fc25957c932 Co-authored-by: Amp <amp@ampcode.com>
📝 WalkthroughWalkthroughRefactors many internal class members from TypeScript private-field syntax ( Changes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/30/2026, 12:37:00 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 26 kB (baseline 26 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 974 kB (baseline 974 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.89 kB (baseline 2.89 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +754 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.1 MB (baseline 7.1 MB) • 🟢 -198 BBundles that do not match a named category
Status: 34 added / 34 removed |
coderabbitai
bot
requested review from
AustinMroz,
Myestery,
christian-byrne and
simula-r
dosubot
bot
added
the
size:XXL
![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
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 324-331: The setter subgraph incorrectly uses this._subgraph after
it's been updated for the 'litegraph:set-graph' dispatch; capture the previous
value into a temp (e.g., const previous = this._subgraph) before assigning
this._subgraph = value, then dispatch with oldGraph: previous and newGraph:
value so listeners receive the correct old and new graphs (references: setter
subgraph, property _subgraph, and the 'litegraph:set-graph' dispatch payload
keys oldGraph/newGraph).
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 3958-3976: The _measureSlots method currently returns null when
there are no measured slots; change it to return undefined instead to follow
project conventions—update the return statement in LGraphNode::_measureSlots so
it returns undefined (or simply omits a return) when slots.length is falsy,
leaving the rest of the logic (calls to this._measureSlot and
createBounds(slots, 0)) unchanged so callers that check truthiness keep working.
| set subgraph(value: Subgraph | undefined) { | ||
| if (value !== this.#subgraph) { | ||
| this.#subgraph = value | ||
| if (value !== this._subgraph) { | ||
| this._subgraph = value | ||
| if (value) | ||
| this.dispatch('litegraph:set-graph', { | ||
| oldGraph: this.#subgraph, | ||
| oldGraph: this._subgraph, | ||
| newGraph: value | ||
| }) |
There was a problem hiding this comment.
Fix oldGraph in the litegraph:set-graph event payload.
oldGraph is read after _subgraph is updated, so listeners get the new graph as the old one. Capture the previous value before assignment.
💡 Proposed fix
set subgraph(value: Subgraph | undefined) {
- if (value !== this._subgraph) {
- this._subgraph = value
- if (value)
- this.dispatch('litegraph:set-graph', {
- oldGraph: this._subgraph,
- newGraph: value
- })
- }
+ if (value !== this._subgraph) {
+ const previous = this._subgraph
+ this._subgraph = value
+ if (value)
+ this.dispatch('litegraph:set-graph', {
+ oldGraph: previous,
+ newGraph: value
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set subgraph(value: Subgraph | undefined) { | |
| if (value !== this.#subgraph) { | |
| this.#subgraph = value | |
| if (value !== this._subgraph) { | |
| this._subgraph = value | |
| if (value) | |
| this.dispatch('litegraph:set-graph', { | |
| oldGraph: this.#subgraph, | |
| oldGraph: this._subgraph, | |
| newGraph: value | |
| }) | |
| set subgraph(value: Subgraph | undefined) { | |
| if (value !== this._subgraph) { | |
| const previous = this._subgraph | |
| this._subgraph = value | |
| if (value) | |
| this.dispatch('litegraph:set-graph', { | |
| oldGraph: previous, | |
| newGraph: value | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraphCanvas.ts` around lines 324 - 331, The setter
subgraph incorrectly uses this._subgraph after it's been updated for the
'litegraph:set-graph' dispatch; capture the previous value into a temp (e.g.,
const previous = this._subgraph) before assigning this._subgraph = value, then
dispatch with oldGraph: previous and newGraph: value so listeners receive the
correct old and new graphs (references: setter subgraph, property _subgraph, and
the 'litegraph:set-graph' dispatch payload keys oldGraph/newGraph).
| private _measureSlots(): ReadOnlyRect | null { | ||
| const slots: (NodeInputSlot | NodeOutputSlot)[] = [] | ||
|
|
||
| for (const [slotIndex, slot] of this.#concreteInputs.entries()) { | ||
| for (const [slotIndex, slot] of this._concreteInputs.entries()) { | ||
| // Unrecognized nodes (Nodes with error) has inputs but no widgets. Treat | ||
| // converted inputs as normal inputs. | ||
| /** Widget input slots are handled in {@link layoutWidgetInputSlots} */ | ||
| if (this.widgets?.length && isWidgetInputSlot(slot)) continue | ||
|
|
||
| this.#measureSlot(slot, slotIndex, true) | ||
| this._measureSlot(slot, slotIndex, true) | ||
| slots.push(slot) | ||
| } | ||
| for (const [slotIndex, slot] of this.#concreteOutputs.entries()) { | ||
| this.#measureSlot(slot, slotIndex, false) | ||
| for (const [slotIndex, slot] of this._concreteOutputs.entries()) { | ||
| this._measureSlot(slot, slotIndex, false) | ||
| slots.push(slot) | ||
| } | ||
|
|
||
| return slots.length ? createBounds(slots, 0) : null | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return undefined instead of null from _measureSlots.
Callers already branch on truthiness, so returning undefined aligns with litegraph conventions without changing behavior.
♻️ Proposed diff
- private _measureSlots(): ReadOnlyRect | null {
+ private _measureSlots(): ReadOnlyRect | undefined {
const slots: (NodeInputSlot | NodeOutputSlot)[] = []
@@
- return slots.length ? createBounds(slots, 0) : null
+ return slots.length ? createBounds(slots, 0) : undefined
}🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraphNode.ts` around lines 3958 - 3976, The
_measureSlots method currently returns null when there are no measured slots;
change it to return undefined instead to follow project conventions—update the
return statement in LGraphNode::_measureSlots so it returns undefined (or simply
omits a return) when slots.length is falsy, leaving the rest of the logic (calls
to this._measureSlot and createBounds(slots, 0)) unchanged so callers that check
truthiness keep working.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/ui/components/popup.ts (1)
92-112: Consider fixing the type annotations (optional, outside PR scope).The
@ts-expect-errorcomments suppress type errors that could be fixed by properly typing the event parameters. As per coding guidelines,@ts-expect-errorshould be avoided in favor of fixing the underlying type issue.♻️ Proposed fix to remove `@ts-expect-error`
- // `@ts-expect-error` fixme ts strict error - private _escHandler = (e) => { + private _escHandler = (e: KeyboardEvent) => { if (e.key === 'Escape') { this.open = false e.preventDefault() e.stopImmediatePropagation() } } - // `@ts-expect-error` fixme ts strict error - private _clickHandler = (e) => { - /** `@type` {any} */ - const target = e.target + private _clickHandler = (e: MouseEvent) => { + const target = e.target as Node | null if ( - !this.element.contains(target) && + target && + !this.element.contains(target) && this.ignoreTarget && !this.target.contains(target) ) { this.open = false } }
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 453-463: The maximumFps getter in LGraphCanvas currently returns
seconds-per-frame because it computes _maximumFrameGap / 1000; change it to
return FPS by inverting the millisecond gap: when _maximumFrameGap >
Number.EPSILON return 1000 / this._maximumFrameGap, otherwise return 0; keep the
setter as-is (it already sets _maximumFrameGap = value > Number.EPSILON ? 1000 /
value : 0). Also update the other occurrence noted (around the other maximumFps
block) to the same logic so both getters consistently return FPS.
In `@src/lib/litegraph/src/serialization.test.ts`:
- Around line 1-3: Move the LiteGraph test file from its current location to the
designated test folder and update its imports accordingly: relocate the file
containing the test that imports LGraph, LGraphNode, and LiteGraph into the
src/lib/litegraph/test/ directory, then adjust the import paths for LGraph,
LGraphNode, and LiteGraph (from './litegraph' to the correct relative path) so
the test resolves modules from the new location and CI/test runner picks it up.
| private _maximumFrameGap = 0 | ||
| /** Maximum frames per second to render. 0: unlimited. Default: 0 */ | ||
| public get maximumFps() { | ||
| return this.#maximumFrameGap > Number.EPSILON | ||
| ? this.#maximumFrameGap / 1000 | ||
| return this._maximumFrameGap > Number.EPSILON | ||
| ? this._maximumFrameGap / 1000 | ||
| : 0 | ||
| } | ||
|
|
||
| public set maximumFps(value) { | ||
| this.#maximumFrameGap = value > Number.EPSILON ? 1000 / value : 0 | ||
| this._maximumFrameGap = value > Number.EPSILON ? 1000 / value : 0 | ||
| } |
There was a problem hiding this comment.
maximumFps getter returns seconds-per-frame instead of FPS.
_maximumFrameGap is treated as milliseconds between frames in the render loop, so the getter should invert that value to expose FPS.
🛠️ Proposed fix
public get maximumFps() {
- return this._maximumFrameGap > Number.EPSILON
- ? this._maximumFrameGap / 1000
- : 0
+ return this._maximumFrameGap > Number.EPSILON
+ ? 1000 / this._maximumFrameGap
+ : 0
}Also applies to: 2082-2085
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraphCanvas.ts` around lines 453 - 463, The maximumFps
getter in LGraphCanvas currently returns seconds-per-frame because it computes
_maximumFrameGap / 1000; change it to return FPS by inverting the millisecond
gap: when _maximumFrameGap > Number.EPSILON return 1000 / this._maximumFrameGap,
otherwise return 0; keep the setter as-is (it already sets _maximumFrameGap =
value > Number.EPSILON ? 1000 / value : 0). Also update the other occurrence
noted (around the other maximumFps block) to the same logic so both getters
consistently return FPS.
| import { describe, expect, it } from 'vitest' | ||
|
|
||
| import { LGraph, LGraphNode, LiteGraph } from './litegraph' |
There was a problem hiding this comment.
Relocate LiteGraph tests to the designated test directory.
This new suite is under src/lib/litegraph/src, but LiteGraph-specific tests are expected in src/lib/litegraph/test/. Please move it there and update imports if needed.
Based on learnings: Applies to src/lib/litegraph/test/** : Place Litegraph-specific tests in src/lib/litegraph/test/.
🧰 Tools
🪛 ESLint
[error] 1-1: Resolve error: EACCES: permission denied, open '/flTiJtKKkm'
at Object.writeFileSync (node:fs:2409:20)
at l (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:13670)
at createFilesMatcher (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:14422)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:70:65)
at Object.resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:147:20)
at file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:69
at setRuleContext (/home/jailuser/git/node_modules/.pnpm/eslint-import-context@0.1.9_unrs-resolver@1.11.1/node_modules/eslint-import-context/lib/index.js:23:20)
at fullResolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:30)
at relative (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:215:12)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:220:16)
(import-x/no-unresolved)
[error] 1-1: Resolve error: EACCES: permission denied, open '/iJxVOgyKil'
at Object.writeFileSync (node:fs:2409:20)
at l (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:13670)
at createFilesMatcher (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:14422)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:70:65)
at Object.resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:147:20)
at file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:69
at setRuleContext (/home/jailuser/git/node_modules/.pnpm/eslint-import-context@0.1.9_unrs-resolver@1.11.1/node_modules/eslint-import-context/lib/index.js:23:20)
at fullResolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:30)
at relative (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:215:12)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:220:16)
(import-x/no-relative-packages)
[error] 1-1: Resolve error: EACCES: permission denied, open '/OowACkYMGx'
at Object.writeFileSync (node:fs:2409:20)
at l (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:13670)
at createFilesMatcher (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:14422)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:70:65)
at Object.resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:147:20)
at file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:69
at setRuleContext (/home/jailuser/git/node_modules/.pnpm/eslint-import-context@0.1.9_unrs-resolver@1.11.1/node_modules/eslint-import-context/lib/index.js:23:20)
at fullResolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:30)
at relative (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:215:12)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:220:16)
(import-x/no-useless-path-segments)
[error] 1-1: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
[error] 3-3: Unable to resolve path to module './litegraph'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/serialization.test.ts` around lines 1 - 3, Move the
LiteGraph test file from its current location to the designated test folder and
update its imports accordingly: relocate the file containing the test that
imports LGraph, LGraphNode, and LiteGraph into the src/lib/litegraph/test/
directory, then adjust the import paths for LGraph, LGraphNode, and LiteGraph
(from './litegraph' to the correct relative path) so the test resolves modules
from the new location and CI/test runner picks it up.
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Only concern previously was with shadowing properties added by 3P code. Using underscore prefix should solve that. See no other issues. LGTM
| override toJSON(): INodeInputSlot { | ||
| return { | ||
| ...super.toJSON(), | ||
| link: this.link, | ||
| widget: this.widget | ||
| } | ||
| } |
There was a problem hiding this comment.
Were there other things you considered for this?
There was a problem hiding this comment.
I was pondering the same. Would it break things if we made all the toJSON calls just return serialize?
There was a problem hiding this comment.
Yes, a few things.
The best solution would be to remove the circular references, but that would be a much larger undertaking and likely™️ cause issues with existing extensions.
Another option is to replace our internal calls to JSON.stringify with a serialize that does much the same thing here, but that has the same issue of not working for existing external callers who would then hit the circular dereference bug at runtime.
There was a problem hiding this comment.
@AustinMroz We'd need to implement serialize on each of these still, then call it from toJSON, right?
There was a problem hiding this comment.
🤔 Probably best we don't implement new serialize methods. The smaller places you added it (like NodeSlot) won't have toJSON called if the parent classes are using their (already safe) serialize methods. And your newly written toJSON methods are still an improvement when debugging with console.log
Fixes a bug where canvas functionality is lost if a multitype input (like the native switch) is added to the graph in litegraph mode. This issue is properly fixed by #8440, but this single revision is easier and safer to backport ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8477-Revert-matchtype-slot-reactivity-on-cloud-1-38-2f86d73d365081279285e6d86ded9e43) by [Unito](https://www.unito.io)
…xy compatibility (#8440) ## Summary Migrates ECMAScript private fields (`#`) to TypeScript private (`private`) across LiteGraph to fix Vue Proxy reactivity incompatibility. ## Problem ES private fields (`#field`) are incompatible with Vue's Proxy-based reactivity system - accessing `#field` through a Proxy throws `TypeError: Cannot read private member from an object whose class did not declare it`. ## Solution - Converted all `#field` to `private _field` across 10 phases - Added `toJSON()` methods to `LGraph`, `NodeSlot`, `NodeInputSlot`, and `NodeOutputSlot` to prevent circular reference errors during serialization (TypeScript private fields are visible to `JSON.stringify` unlike true ES private fields) - Made `DragAndScale.element.data` non-enumerable to break canvas circular reference chain ## Testing - All 4027 unit tests pass - Added 9 new serialization tests to catch future circular reference issues - Browser tests (undo/redo, save workflows) verified working ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8440-refactor-migrate-ES-private-fields-to-TypeScript-private-for-Vue-Proxy-compatibility-2f76d73d365081a3bd82d429a3e0fcb7) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
…xy compatibility (#8440) ## Summary Migrates ECMAScript private fields (`#`) to TypeScript private (`private`) across LiteGraph to fix Vue Proxy reactivity incompatibility. ## Problem ES private fields (`#field`) are incompatible with Vue's Proxy-based reactivity system - accessing `#field` through a Proxy throws `TypeError: Cannot read private member from an object whose class did not declare it`. ## Solution - Converted all `#field` to `private _field` across 10 phases - Added `toJSON()` methods to `LGraph`, `NodeSlot`, `NodeInputSlot`, and `NodeOutputSlot` to prevent circular reference errors during serialization (TypeScript private fields are visible to `JSON.stringify` unlike true ES private fields) - Made `DragAndScale.element.data` non-enumerable to break canvas circular reference chain ## Testing - All 4027 unit tests pass - Added 9 new serialization tests to catch future circular reference issues - Browser tests (undo/redo, save workflows) verified working ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8440-refactor-migrate-ES-private-fields-to-TypeScript-private-for-Vue-Proxy-compatibility-2f76d73d365081a3bd82d429a3e0fcb7) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
3 participants
Summary
Migrates ECMAScript private fields (
#) to TypeScript private (private) across LiteGraph to fix Vue Proxy reactivity incompatibility.Problem
ES private fields (
#field) are incompatible with Vue's Proxy-based reactivity system - accessing#fieldthrough a Proxy throwsTypeError: Cannot read private member from an object whose class did not declare it.Solution
#fieldtoprivate _fieldacross 10 phasestoJSON()methods toLGraph,NodeSlot,NodeInputSlot, andNodeOutputSlotto prevent circular reference errors during serialization (TypeScript private fields are visible toJSON.stringifyunlike true ES private fields)DragAndScale.element.datanon-enumerable to break canvas circular reference chainTesting
┆Issue is synchronized with this Notion page by Unito