Fix: Removing not null from validation#6298
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the validateFlowData function by relaxing parameter types to any[] and removing non-null assertions from node lookups. The review feedback highlights a regression in type safety and potential runtime errors caused by accessing properties on potentially undefined results from the find method. It is recommended to revert the type changes and either restore the non-null assertions or add explicit safety checks.
| edges: IReactFlowEdge[], | ||
| componentNodes: IComponentNodes | ||
| ): IValidationResult[] => { | ||
| export const validateFlowData = (nodes: any[], edges: any[], componentNodes: IComponentNodes): IValidationResult[] => { |
There was a problem hiding this comment.
Changing the parameter types from IReactFlowNode[] and IReactFlowEdge[] to any[] is a regression in type safety. These interfaces provide essential structure and help prevent runtime errors. If the goal was to avoid TypeScript errors after removing the non-null assertions, it's better to address the nullability directly rather than resorting to any.
| export const validateFlowData = (nodes: any[], edges: any[], componentNodes: IComponentNodes): IValidationResult[] => { | |
| export const validateFlowData = (nodes: IReactFlowNode[], edges: IReactFlowEdge[], componentNodes: IComponentNodes): IValidationResult[] => { |
References
- Using specific interfaces for function parameters is preferred over 'any' to maintain type safety and provide clear documentation of the expected data structure.
| if (!sourceExists && targetExists) { | ||
| // Target exists but source doesn't - add issue to target node | ||
| const targetNode = nodes.find((node: IReactFlowNode) => node.id === edge.target)! | ||
| const targetNode = nodes.find((node: IReactFlowNode) => node.id === edge.target) |
There was a problem hiding this comment.
Removing the non-null assertion (!) while still accessing properties like targetNode.id on line 264 is unsafe. If nodes.find returns undefined, the application will crash at runtime. Since the existence of the node is already verified by targetExists on line 249, using the ! operator is the correct way to inform TypeScript of this guarantee. If you prefer to avoid !, you should add an explicit check for targetNode before accessing its properties.
| const targetNode = nodes.find((node: IReactFlowNode) => node.id === edge.target) | |
| const targetNode = nodes.find((node: IReactFlowNode) => node.id === edge.target)! |
| } else if (sourceExists && !targetExists) { | ||
| // Source exists but target doesn't - add issue to source node | ||
| const sourceNode = nodes.find((node: IReactFlowNode) => node.id === edge.source)! | ||
| const sourceNode = nodes.find((node: IReactFlowNode) => node.id === edge.source) |
There was a problem hiding this comment.
Similar to the target node lookup, removing the non-null assertion here without a corresponding null check makes the property access on line 281 (sourceNode.id) potentially unsafe. It is recommended to keep the ! operator since sourceExists (line 248) confirms the node's presence, or implement a proper guard clause.
| const sourceNode = nodes.find((node: IReactFlowNode) => node.id === edge.source) | |
| const sourceNode = nodes.find((node: IReactFlowNode) => node.id === edge.source)! |
|
Not Needed. |
Removing
!since we don't want to enforce not null behavior and keeping it inline with the original code.