Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

batch listener notifications #11

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dehli
Copy link
Collaborator

@dehli dehli commented Jun 14, 2024

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-tick to 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!

@dehli dehli force-pushed the dehli/listener-ordering branch 2 times, most recently from 68e83fa to 5b06de5 Compare June 14, 2024 15:56
@dehli dehli changed the title order listener invocation group listener notifications Jun 14, 2024
core/src/refx/subs.cljc Outdated Show resolved Hide resolved
@@ -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)}]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched the key so it can be easily sortable since use-sub-11 would come before use-sub-2 when using default string comparison.

@dehli dehli changed the title group listener notifications batch listener notifications Jun 15, 2024
@dehli dehli marked this pull request as ready for review June 18, 2024 12:21
(remove-listeners!)
(async done
(js/setTimeout (fn []
(is (= @listener-calls
Copy link
Collaborator Author

@dehli dehli Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dehli
Copy link
Collaborator Author

dehli commented Jul 11, 2024

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 useExternalStore is being triggered one time after -remove-listener is called.

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 dehli force-pushed the dehli/listener-ordering branch 2 times, most recently from 8cb117c to 7a71115 Compare July 11, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant