batch listener notifications #11
Conversation
dehli
force-pushed
the
dehli/listener-ordering
branch
2 times, most recently
from
68e83fa to
5b06de5
Compare
dehli
force-pushed
the
dehli/listener-ordering
branch
from
5b06de5 to
b528e49
Compare
core/src/refx/hooks.cljs
Outdated
| @@ -28,7 +28,7 @@ | |||
| (react/useMemo | |||
| (fn [] | |||
| [(fn [callback] | |||
| (let [key (str "use-sub-" (swap! use-sub-counter inc))] | |||
| (let [key {:index (swap! use-sub-counter inc)}] | |||
There was a problem hiding this comment.
Switched the key so it can be easily sortable since use-sub-11 would come before use-sub-2 when using default string comparison.
| (remove-listeners!) | ||
| (async done | ||
| (js/setTimeout (fn [] | ||
| (is (= @listener-calls |
There was a problem hiding this comment.
Without these changes, this test fails and listener-calls has a value of:
[{::subs/index 4}
{::subs/index 3}
{::subs/index 1}
{::subs/index 4}
{::subs/index 2}]Note that order isn't guaranteed and 4 occurs twice which would translate to an extra render.
|
Wanted to follow up and surface that there might be a minor bug with this. After using this fork a bit, I'm seeing that I'm calling a single update on an unmounted component when changing routes and I believe it's because Will confirm that it is related to these changes, and if so I'll get another commit pushed up to address. Edit: have confirmed that this fork introduced the issue. Added a commit (90e02dd) to address the issue and am no longer seeing the React warnings. |
dehli
force-pushed
the
dehli/listener-ordering
branch
2 times, most recently
from
8cb117c to
7a71115
Compare
dehli
force-pushed
the
dehli/listener-ordering
branch
from
7a71115 to
90e02dd
Compare
Hello again 👋 This PR addresses #7. I like this approach quite a bit more than my earlier approach as it's isolated to the listener implementation. With this change, I use an atom along with
interop/next-tickto ensure that all "ui" listeners (from the use-sub hook) are triggered after values have updated (both Sub and DynamicSub updates).The listeners are fired in the order that they are registered (which is why I changed the key for
use-sub). This is important because components higher in the tree (registered earlier) should have their hooks updated before further down components. An example of why this is needed is if you had a router subscribe to the current route, you'd want to make sure it's updated before downstream components are (otherwise downstream components could render, when they shouldn't, and have invalid hooks fired right before unmounting).I've added these changes to my own refx project and it's been working great for the past week (I'm seeing improved performance when using our app 🎉).
Let me know if you'd like any changes or have any feedback!