-
Notifications
You must be signed in to change notification settings - Fork 491
Road to No Explicit Any Part 8 (Group4) #8314
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
Changes from all commits
f923c41
e9e4780
8f3e600
154e631
8a447b5
f2ebd59
ab5d356
bfcd642
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2603,6 +2603,10 @@ export class Subgraph | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| addInput(name: string, type: string): SubgraphInput { | ||||||||||||||
| if (name === null || type === null) { | ||||||||||||||
| throw new Error('Name and type are required for subgraph input') | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| this.events.dispatch('adding-input', { name, type }) | ||||||||||||||
|
|
||||||||||||||
| const input = new SubgraphInput( | ||||||||||||||
|
|
@@ -2621,6 +2625,10 @@ export class Subgraph | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| addOutput(name: string, type: string): SubgraphOutput { | ||||||||||||||
| if (name === null || type === null) { | ||||||||||||||
| throw new Error('Name and type are required for subgraph output') | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+2628
to
+2630
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue: This check mirrors 🐛 Proposed fix addOutput(name: string, type: string): SubgraphOutput {
- if (name === null || type === null) {
+ if (name == null || type == null) {
throw new Error('Name and type are required for subgraph output')
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| this.events.dispatch('adding-output', { name, type }) | ||||||||||||||
|
|
||||||||||||||
| const output = new SubgraphOutput( | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { describe, expect, it, vi } from 'vitest' | |||||||||
|
|
||||||||||
| import type { SubgraphNode } from '@/lib/litegraph/src/litegraph' | ||||||||||
| import { LGraph, Subgraph } from '@/lib/litegraph/src/litegraph' | ||||||||||
| import type { SubgraphInput } from '@/lib/litegraph/src/subgraph/SubgraphInput' | ||||||||||
|
|
||||||||||
| import { subgraphTest } from './__fixtures__/subgraphFixtures' | ||||||||||
| import { | ||||||||||
|
|
@@ -531,7 +532,7 @@ describe.skip('SubgraphNode Cleanup', () => { | |||||||||
|
|
||||||||||
| // Now trigger an event - only node1 should respond | ||||||||||
| subgraph.events.dispatch('input-added', { | ||||||||||
| input: { name: 'test', type: 'number', id: 'test-id' } as any | ||||||||||
| input: { name: 'test', type: 'number', id: 'test-id' } as SubgraphInput | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider using The mock object subgraph.events.dispatch('input-added', {
- input: { name: 'test', type: 'number', id: 'test-id' } as SubgraphInput
+ input: { name: 'test', type: 'number', id: 'test-id' } as Partial<SubgraphInput> as SubgraphInput
})This makes it clear the mock is intentionally incomplete. Based on learnings, this pattern is preferred for test mocks. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| }) | ||||||||||
|
|
||||||||||
| // Only node1 should have added an input | ||||||||||
|
|
@@ -558,7 +559,7 @@ describe.skip('SubgraphNode Cleanup', () => { | |||||||||
|
|
||||||||||
| // Trigger an event - no nodes should respond | ||||||||||
| subgraph.events.dispatch('input-added', { | ||||||||||
| input: { name: 'test', type: 'number', id: 'test-id' } as any | ||||||||||
| input: { name: 'test', type: 'number', id: 'test-id' } as SubgraphInput | ||||||||||
Myestery marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| }) | ||||||||||
|
|
||||||||||
| // Without cleanup: all 3 removed nodes would have added an input | ||||||||||
|
|
||||||||||
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.
Runtime validation does not catch
undefinedvalues.The check
name === null || type === nullonly catchesnull, but the tests expect bothnullandundefinedto throw (seeSubgraphEdgeCases.test.tslines 114-123). Sinceundefined === nullevaluates tofalse, passingundefinedwill not trigger the error.🐛 Proposed fix to also check for undefined
addInput(name: string, type: string): SubgraphInput { - if (name === null || type === null) { + if (name == null || type == null) { throw new Error('Name and type are required for subgraph input') }Using loose equality (
==) catches bothnullandundefinedin a single check.🤖 Prompt for AI Agents