Update react reconciler

Yomguithereal / react-blessed

A react renderer for blessed.

MIT License

4.45k stars 177 forks source link

Update react reconciler #116

Closed speakingcode closed 3 years ago

speakingcode commented 3 years ago

Per #115, react-blessed needs to update to new react-reconciler to avoid bugs related to flushing passive effects (for example causing react-redux subscriptions to clean up when they should not). This required some changes to the HostConfig. I also updated the "test" script entry in package.json and the .babelrc config so that mocha would properly run tests with current babel, with module support.

npm test passes and manually checking demos works as well.

Yomguithereal commented 3 years ago

Ok @speakingcode. Let met merge this then, check some things and release as v0.7.0 on npm soon.

Yomguithereal commented 3 years ago

Ok I have found an issue with the remove example. It seems node removal is broken. I am currently investigating.

Yomguithereal commented 3 years ago

I struggle to find a solution. To reproduce, just run npm run demo remove.

Yomguithereal commented 3 years ago

Ok I found a fix as per https://github.com/Yomguithereal/react-blessed/commit/b38c2a75b99cba5e77f7182a036551f1e4000b7c. It was a tricky one to find...

Yomguithereal commented 3 years ago

Will try to upgrade devtools & prepare peer deps for a release soon.

Yomguithereal commented 3 years ago

v0.7.0 is now live on npm

speakingcode commented 3 years ago

Good find! sorry about the overlook of that test, and thanks for accepting the PR!

zcaudate commented 3 years ago

@speakingcode, @Yomguithereal

There's still an issue with this:

The counter example works fine:

but in the game of life example:

both next and start (for the timer) uses a call next_fn
clicking next renders correctly - the counter updates and the grid updates to the next state
clicking start will render the updated counter but not update the grid to the next state

Yomguithereal commented 3 years ago

@zcaudate I am trying to find what could be the issue here but to no avail. Could you try to reproduce the issue: 1. without immer and 2. while wrapping your interval in a useEffect to avoid bypassing react's lifecycle?

zcaudate commented 3 years ago

@Yomguithereal: It actually works when setInterval is put into useEffect. Thanks for the tip.

the repo without immer is here:

https://github.com/zcaudate/play.tui-game-of-life-raw

Yomguithereal commented 3 years ago

So the question would then be: would your example work for instance with react-dom? The underlying question is of course: should I fix something? Can I fix something :) ?

zcaudate commented 3 years ago

I think it is fixed. I'll try it out with react-dom in a couple of days but I think the problem was use of setInterval outside of react lifecycle.

Yomguithereal commented 3 years ago

Fair enough. Keep me informed when you do so :)