Skip to content

refactor(flow): rebuild layout algorithm from scratch#430

Open
nandanmen wants to merge 6 commits intocloudflare:mainfrom
nandanmen:nanda/flow-rewrite
Open

refactor(flow): rebuild layout algorithm from scratch#430
nandanmen wants to merge 6 commits intocloudflare:mainfrom
nandanmen:nanda/flow-rewrite

Conversation

@nandanmen
Copy link
Copy Markdown
Contributor

This PR rebuilds Flow to use a bespoke layout algorithm instead of relying on the DOM's layout algorithm.

Problem

The existing Flow implementation relies on the DOM to layout the nodes in the diagram, which keeps things simple and allows us to position nodes using CSS.

The problem is the arrows that we draw between nodes need explicit coordinates. To get these coordinates from the DOM, we need to:

  • Measure and store the size and position of every node
  • Update size and position every time either size or position changes

We can easily check for size changes using ResizeObserver, but we can't do the same for position. As a result, we have to do various hacks to ensure position always stay in sync:

  • Updates on scroll and resize
  • Measurement epochs to mass update groups of nodes

This led to a few prevailing issues:

  1. Mass re-renders because we can never be sure if a position is stale
  2. Positions becoming stale anyway
  3. Animations becoming practically infeasible because of performance issues from (1)

Solution

This PR attempts to fix the problem by capturing control of how the nodes are laid out. It implements a custom layout algorithm that takes the size of each node and returns their x and y coordinates. These positions are based on where the node is rendered in the component tree.

By manually computing positions, we can:

  1. Guarantee that node positions will never go stale
  2. Reduce recomputation to only style changes
  3. Support animations and transitions

  • Reviews
    • bonk has reviewed the change
    • automated review not possible because:
  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant