Fork me on GitHub
#hyperfiddle
<
2023-03-24
>
mattias13:03:59

I'm encountering two issues in https://github.com/subspace-ai/subspace: 1. There is a dom/div https://github.com/subspace-ai/subspace/blob/d15601d3c04c4a07f9c01eb52b663508a8432706/src/app/nodes.cljc#L30 (which has :contenteditable true ) with a dom/text https://github.com/subspace-ai/subspace/blob/d15601d3c04c4a07f9c01eb52b663508a8432706/src/app/nodes.cljc#L33 nested within. If the initial value of dom/text is empty or whitespace, then the corresponding DOM element does not get shown in Chrome DevTools inspector (perhaps the browser cleans it out, or Electric does). Then if I edit the contents of the div in the browser to "val", and focus away from the div (which triggers an https://github.com/subspace-ai/subspace/blob/d15601d3c04c4a07f9c01eb52b663508a8432706/src/app/nodes.cljc#L59 of the corresponding DB record), then the DOM tree ends up with two text nodes with content "val". If I focus back to the div and away again, the text nodes become "val" and "valval" . I think it's because the first text node "val" is added there by Chrome's contenteditable behaviour and is ignored by Electric, and the second value "valval" is the dom/text node that Electric adds, since now that the DB gets a new non-empty value, the text node gets added and updated (it becomes "valval" because the DB update from last time used both text nodes of the div). If the initial value of the dom/text node is non-empty, everything works fine. 2. When https://github.com/subspace-ai/subspace/blob/d15601d3c04c4a07f9c01eb52b663508a8432706/src/app/nodes.cljc#L53, I'm seeing caught TypeError: Cannot read properties of null (reading 'removeChild') after the backend syncs the changes to the frontend, and the DOM elements (<li><div>) corresponding to the Node don't get cleaned out. Presumably it's expecting an entry for the now-deleted node, which is nil?

2
Dustin Getz18:03:47

after first read, it seems like there is some interaction with contenteditable (we had not tried this yet, you are the first to try). This would not be surprising as we have seen issues with elements being mutated in unexpected ways (i.e. adding another textnode that we didn't account for). I will take a closer look when i have time - tonight or tomorrow hopefully. I'll look at #2 as well, it could even be related

👍 2
Dustin Getz13:03:31

Ah, #2 is fixed in master already, which hasn't been released yet on maven, you can use :git/url :git/sha

Dustin Getz14:03:45

Confirmed #2 is fixed and sent PR; and the fact that the master change fixes it also confirms that there's something funky going on with how contenteditable mutates the DOM for issue #1, investigating

👍 2
Dustin Getz16:03:37

the first PR I sent (that upgraded Electric to master) is no longer needed, as the content-editable was the root cause, so with that fixed you don't need to upgrade

Dustin Getz16:03:59

Also note we need to use an optimistic state pattern here, because we want local updates to not be delayed by round trip latency. Let's worry about that later (you will need it to deploy). We have components for this but you should wait until we release the next version of Electric in about 2 weeks which has changes here

mattias05:03:46

Amazing, thank you so much for the PR and explanations!

🙂 2