nodesInitialized will fire before node measurements are exposed since xyflow 12-13

collinsleewyatt commented 3 weeks ago

Describe the Bug

When using React Flow v12.13, useNodesInitialized appears to fire before getNodes() is able to expose its .measured values. Additionally, a useEffect(() => {}, [nodes]) will no longer fire after the nodes get measured as of v12.13.

Your Example Website or App

See codesandboxes below.

Steps to Reproduce the Bug or Issue

For v12-next.12 (expected behavior), go here: https://codesandbox.io/p/sandbox/reactflow-measurement-bug-v12-next-12-ms4gx9 For v12-next.13 (unexpected behavior), go here: https://codesandbox.io/p/sandbox/reactflow-measurement-bug-v12-next-13-thgclp

On both: Pressing the "Add node" button will change the nodes array. Pressing the "Remove nodes" button will remove all nodes.

Press "Add node" on both, and watch for the alerts.

The first useEffect calls an alert when nodesInitialized changes ("Nodes have initialized properly." / "Nodes say they are initialized but do not have a defined measurement"). We say nodes have initialized properly if nodesInitialized is true AND getNodes().every((node) => node.measured !== undefined || node.computed !== undefined) is also true, which I would expect (keep in mind v12.13 changed the computed field to measured).
The second calls an alert when the nodes array changes in any way ("nodes change detection"). We'd expect two alerts after calling setNodes with new nodes: one for when we change the array ourselves, and one for when the new nodes get measured (our demo is set up to always create a new node)

Expected behavior

On v12.12, the events fired will be (in order):

nodes change detection, which detects us changing the nodes by hitting the button.
nodes initialized properly, which detects useNodesInitialized changed properly at the right time.
nodes change detection, which detects React Flow's change to add measurements.

On v12.13, this is different:

nodes change detection, which detects us changing the nodes by hitting the button.
nodes initialized improperly, which detects useNodesInitialized fired but the measured field was undefined.

As you can see, the third event, nodes change detection, no longer fires when React Flow measures the nodes, and nodesInitialized changed to true before its measurements were exposed, meaning we could not get the dimensions.

Screenshots or Videos

https://github.com/xyflow/xyflow/assets/54917619/6eed49cf-34fe-44da-8889-afdbd9611fe3

Platform

OS: macOS on M1
Browser: Safari 17.4, Chrome 124

Additional context

No response

moklick commented 3 weeks ago

Thanks for the super detailed explanation here :) This should be fixed in 12.0.0-next.16.

collinsleewyatt commented 3 weeks ago

Hey, I've tested my example on v12-next.16, and I'm not able to get nodesInitialized to run consistently on the demo. I've added an alert statement whenever nodesInitialized changes for debugging purposes. The nodes useEffect will run only once (not twice for population & measurement), and the nodesInitialized useEffect seems to only fire when I initialize / hot reload the page. However, on my school project, I'm able to get nodesInitialized to fire, and I'm not really sure why it's inconsistent here.

This is on v12-next.16: https://codesandbox.io/p/sandbox/reactflow-measurement-bug-v12-next-12-forked-yw88qs

Thanks for the response and maintaining this package.

moklick commented 3 weeks ago

It seems we need to re-open this :/ We will check it next week. Thanks for the codesandbox!

collinsleewyatt commented 3 weeks ago

Thanks! Take care.

moklick commented 2 weeks ago

This is fixed in 12.0.0-next.17. Thanks again @collinsleewyatt !

collinsleewyatt commented 2 weeks ago

Thanks :) I can confirm that behavior is consistent between 12.0.0-next.12 and 12.0.0-next.17. Appreciate the look into this.

xyflow / xyflow