Bug: DecoratorNode's component does not call useEffect cleanup until after remount

[adri1wald]

DecoratorNode.decorate can return JSX element to render React components. Surprisingly, when you delete a DecoratorNode, useEffect cleanup functions are not called. Even more surprisingly, when you undo the delete, the useEffect cleanup function is called.

Lexical version: 0.4.1

Steps To Reproduce

1. Clone https://github.com/adri1wald/lexical-decorator-node-useeffect-cleanup-repro
2. Follow steps outlined in README

The current behavior

Cleanup doesn't happen when DecoratorNode is delete. It happens if and when the DecoratorNode is remounted.

The expected behavior

Cleanup should happen at the expected point in the component lifecycle, i.e. when it unmounts / when it looks like it unmounts.

[trueadm]

Ah, I think I might know why this is happening.

https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalUpdates.ts#L511

We are capturing the pendingDecorators before we garbage collect any decorators. Doh.

We should probably move this line down to where we actually use it.

https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalUpdates.ts#L566

Feel free to put up a PR if you'd like to contribute to Lexical :) thanks for finding the issue too!

[adri1wald]

Not sure where your React 18 comment went but I tested with React 18 in the repro and same issue. Will have a look at LexicalUpdates code and see if I can make sense of it :)

[echarles]

I had hard time a few days ago with the decorator node deletion. At some point, in my app, some decorator nodes were deleted whereas it should not be deleted, It was then rendered again. I don't have bandwith for now to setup a reproducer, but though useful to mention that. That behavior can be reproduced in the playground if you console.log in the cleanup function fo a useEffect.

And yes, I have seen that pendingDecorators was in the picture (the first deleted decorator node was removed, then the cleanup functions were called on the second decorator node delettion, with potentially other non deleted nodes by the user being deleted and directly re-rendered (which in my case was giving issues to the state I was maintaining in the decorator nodes).

[trueadm]

I believe the changes I mentioned above should help in both your cases. I’m on parental leave right now and on my phone otherwise I’d put up a PR.