fix/COMPASS-9935 fix callbacks #168
Conversation
paula-stacho
changed the title
| import { EMPLOYEES_NODE, ORDERS_NODE } from '@/mocks/datasets/nodes'; | ||
| import { EMPLOYEES_TO_ORDERS_EDGE } from '@/mocks/datasets/edges'; | ||
|
|
||
| const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
maybe: import { setTimeout as sleep } from 'timers/promises';
| onSelectionChange={onSelectionChange} | ||
| />, | ||
| ); | ||
| await screen.getByText('orders').click(); |
There was a problem hiding this comment.
await seems to be redundant and following ones as well
| fields: getInternalFields(fields), | ||
| borderVariant, | ||
| variant, | ||
| externalNode: node, |
There was a problem hiding this comment.
could this have been where we passed ReactNode to the package and the diagram disappeared? 🤔
There was a problem hiding this comment.
this was just something I added in #167 as I missed the full context on where the external nodes are used. apparently we have some callbacks where instead of passing just the relevant changes, we're passing the whole node - https://github.com/mongodb-js/diagramming/blob/main/src/types/component-props.ts#L73:L103. We're generally ignoring the irrelevant info and not overwriting our nodes, but it's definitely a risky pattern and I'd like to change this. But it will be a breaking change for relational migrator, so I need to sync with them on that.
There was a problem hiding this comment.
If we were overwriting the nodes, this could've explained one node disappearing (the one triggering a callback). The whole diagram disappearing was a reaction to us triggering a number of re-renders, even without any callbacks involved (like us re-centering on a newly added node). That one was definitely on us preventing the memoization of the nodes by including non-serializable items in the node data.
Description
I realised I made a mistake in #167, breaking the callbacks because they were depending on the node to be updated.
This change includes storing the fields with
isVisibleinstead of filtering out the invisible ones right away. This makes the conversion much simpler, we just need to ensure that we check for the visibility for both the field list and height calculations.Adding some tests for the callbacks as there weren't any. Unfortunately I wasn't able to get the drag working in tests, which would've been the most important.