search

Yomguithereal / react-blessed

A react renderer for blessed.
MIT License
4.45k stars 177 forks source link

Events shall be removed for all descendants #101

Closed lins05 closed 4 years ago

lins05 commented 4 years ago

I have experienced a weird problem that even if a component is no longer on the screen, its event handler (a keyboard callback in my case) is still responding to the keyboard events.

The reason for this bug is:

  1. In blessed, Node.destroy won't unbind events for the descendants of the node.
  2. React fiber only calls removeChildFromContainer or removeChild for the top level element being removed (I reached this conclusion by using a . Thus if we have a component <Box><TextBox/></Box> to be removed, the inner TextBox won't have a chance to cleanup its event handlers.

https://github.com/chjj/blessed/blob/eab243fc7ad27f1d2932db6134f7382825ee3488/lib/widgets/node.js#L176-L183

So the fix is to explicitly unbind events for all the descendants of the current element being removed.

Yomguithereal commented 4 years ago

Hello @lins05. Thanks for the PR. Something I don't understand is the contradiction between:

In blessed, Node.destroy won't unbind events for the descendants of the node.

And the link https://github.com/chjj/blessed/blob/eab243fc7ad27f1d2932db6134f7382825ee3488/lib/widgets/node.js#L176-L183 which seems to indicate otherwise.

I think this sentence:

I reached this conclusion by using a .

lacks something.

Anyway, my point is, shouldn't this be something to fix on blessed side? Else I will of course merge so this issue goes away.

lins05 commented 4 years ago

@Yomguithereal Thx for the reply. See my comments below.

which seems to indicate otherwise.

Nope, if you follow the code of Node.prototype.destroy you'll find that the descendants' EventEmitter.prototype.off method is not called anywhere.

I reached this conclusion by using a .

Sorry, I mean "I reached this conclusion by using the chrome node js debugger".

Anyway, my point is, shouldn't this be something to fix on blessed side?

Well, blessed is a de facto abandoned project by the original author, so I guess we have to fix it here ...

Yomguithereal commented 4 years ago

Fair enough @lins05. Let me merge this then. Do you want to try and open a PR on blessed just in case?

Yomguithereal commented 4 years ago

v0.6.2 is now live on npm

lins05 commented 4 years ago

Thx!

Do you want to try and open a PR on blessed just in case?

I'd rather try my luck in neo-blessed, but seems all react-blessed, blessed-contrib, react-blessed-contrib are still based on original blessed, I don't have much motivation to do that ..

lirantal commented 4 years ago

I can update blessed-contrib if necessary.