Specify useLayoutEffect dependency array

[keyneom]

Executing this effect upon every react re-render can cause the focus to be stolen when a user is interacting with other components. Specifying the appropriate dependency array should ensure that the effect is only executed when necessary.

This fixes #16

[keyneom]

@cpresler or @kitten any chance of this making it in? I know you all are in maintenance mode for this repository but this PR should only be a bug fix and adds no additional features. I'm happy to answer any questions you may have regarding the changes as well. Thanks for the work you all do!

[kitten]

The array there has actually been left out on purpose and that's quite an important detail. The code there needs to be executed whenever React updates (i.e. whenever the component that contains this effect re-renders) because otherwise, on render, the observer won't be disconnected and will see any changes that the React update will make.

That means it's basically there on purpose.

That said, this approach will need to be tweaked in React 18, because either the rendering will need to be kicked into synchronous mode (i.e. a React sync lane) or it'll need to be resilient to React's gradual updates.
That's a little unrelated to this, but thought I may add this on

[keyneom]

I'm not sure I follow what you mean by this because otherwise, on render, the observer won't be disconnected and will see any changes that the React update will make. Which react updates would be made that we don't want the observer to be aware of?

My understanding is that react would only run the cleanup code just prior to completing the new rendering cycle (since this is a useLayoutEffect) but by the time mutation has occurred we would have already received that change with the existing observer, then the observer will get disconnected prior to any changes being made to the dom and then observe will be called--again before any changes are made to the dom since useLayoutEffect will rerun it's setup code prior to the render being completed. i.e. I don't think any mutations to the dom are being avoided with the disconnect as it currently stands because disconnect and the subsequent observe call are all happening between changes to the dom.

To be clear, I'm not implying you are wrong, I'm just not clear on what exactly is problematic about the fix so I can make sure to avoid introducing further problems. Would you be able to provide some sample code that demonstrates the issue?

I've forked the codesandbox from the readme to use my modified code (with a console.log of the mutations observed added as well) and compared the observed mutations with the dependency array added vs excluded when typing in the preview and haven't seen a difference in the two scenarios. You can check it out here: https://codesandbox.io/s/use-editable-forked-yz5zcj?file=/src/useEditable.ts:7836-7971

[keyneom]

Actually, in re-checking the docs again, I think I might understand what you are referring to here: https://beta.ja.reactjs.org/reference/react/useLayoutEffect#usage

In the case that the component is rendered multiple times you could see mutations being observed twice, is that correct? I would have thought that it isn't problematic and haven't seen any issues in my use of the changes so far, but I can see how it could be if the mutations are not handled idempotently but I think my post from above still stands in that the new observe call is already made at that point so the duplicate mutations are still observed regardless--I might be wrong on the order of operations there though. I haven't looked into the rest of the code enough to say how the layout render before browser repaint would impact things.