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

Replace react-visibility-sensor with react-intersection-observer #5909

Merged
merged 2 commits into from Jul 6, 2022

Conversation

phoenixeliot
Copy link
Contributor

@phoenixeliot phoenixeliot commented Jul 4, 2022

This attempts to implement the change suggested here: #5270 (comment)

This replaces the (old, unmaintained) react-visibility-sensor with an IntersectionObserver based library, react-intersection-observer.

This is motivated by my finding that the previous package was causing 20% CPU usage at idle, on my computer, from setInterval firing every 100ms and checking visibility statuses.

I think this logic is equivalent to what we were doing with the previous library, but this should get a bit more thorough testing before it's considered done. But it appears to work completely from loading and using the page. If you know a good way to test this logic with the dummy database, please let me know. I'll see if I can come up with anything tomorrow / when I next get around to this if work takes up too much of my brain tomorrow.

Please, let me know if I'm doing anything wrong/weird/unconventionally. I have used LISP before and JS for my work, but never ClojureScript, or Rum, and they're still very very new to me (as of this ticket).

Some things in particular I'm not sure about:

  • Is there a way to destructure objects in ClojureScript? Is there a better way than just referenceing eg (gobj/get inViewState "inView") each time?
  • Why does (gobj/get inViewState "inView") work but (:inView inViewState) doesn't? Is it because the second syntax is only for native clojure map types or so?
  • Is this the correct way to pass a ref to a Rum component? I tried using rum/with-ref but I couldn't for the life of me figure out how to get it to do anything / how to use the value inside the component that is being passed the ref.

Before (while sitting idle in the desktop app):
CleanShot 2022-07-04 at 01 28 18@2x

After (while sitting idle in the desktop app):
CleanShot 2022-07-04 at 01 29 39@2x

@tiensonqin
Copy link
Contributor

tiensonqin commented Jul 6, 2022

@phoenixelio Thanks a lot for this PR! ❤️

I added some comments.

Is there a way to destructure objects in ClojureScript? Is there a better way than just referenceing eg (gobj/get inViewState "inView") each time?

You can access a property using .-, e.g. (.-inView inViewState), another way is to use cljs-bean to convert the js object to a Clojure data structure, e.g. (cljs-bean.core/->clj inViewState) will get you a vector.

Why does (gobj/get inViewState "inView") work but (:inView inViewState) doesn't? Is it because the second syntax is only for native clojure map types or so?

Yes, it's because inViewState is a js object instead of a Clojure map.

Is this the correct way to pass a ref to a Rum component?

Yes, it works because ref here is a function that can be passed to other functions.

This PR works great, the only issue is that the bottom components are not GCed when scrolling upwards, which can slow down editing and increase memory usage. I'll merge your PR first and push another fix. 🚀

(rum/defc lazy-visible
[content-fn]
(let [[hasBeenSeen setHasBeenSeen] (rum/use-state false)
inViewState (useInView {:rootMargin "0px 0px 0px 0px"
Copy link
Contributor

Choose a reason for hiding this comment

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

useInView requires a js object, {:rootMargin ...} needs a #js metadata, e.g. #js {:rootMargin}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ty!

BTW, I think I mean to put this back at "300px 0px 300px 0px" so it loads things a bit before you see them. Didn't expect you to merge this so fast!

[content-fn]
(let [[hasBeenSeen setHasBeenSeen] (rum/use-state false)
inViewState (useInView {:rootMargin "0px 0px 0px 0px"
:on-change (fn [v] (when v (setHasBeenSeen v)))})]
Copy link
Contributor

Choose a reason for hiding this comment

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

onChange is needed here.

(let [[hasBeenSeen setHasBeenSeen] (rum/use-state false)
inViewState (useInView {:rootMargin "0px 0px 0px 0px"
:on-change (fn [v] (when v (setHasBeenSeen v)))})]
(rum/use-effect!
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to add this effect once we addressed the above onChange problem.

@tiensonqin tiensonqin merged commit 3c31448 into logseq:master Jul 6, 2022
@phoenixeliot
Copy link
Contributor Author

phoenixeliot commented Jul 6, 2022

Wow, thanks for such a helpful and speedy response!

This PR works great, the only issue is that the bottom components are not GCed when scrolling upwards, which can slow down editing and increase memory usage. I'll merge your PR first and push another fix. 🚀

One thing I was confused by was that when testing, the old code seemed to not unload things / mark things as invisible even when off the page by a large margin, which seemed like a bug in the library being used. I'm not 100% sure that test result was correct though.

But I kept it like that because if I allowed unloading when off the page, the page would twitch up and down quite a lot as things above the viewport changed their height when their real content was removed and replaced with a placeholder. It can be safe to remove content below the viewport, but above the viewport is much harder.

Perhaps a solution to that could be to make the top-margin extremely large (10M pixels or something that will never be exceeded) but the bottom margin a reasonable 300px

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.

None yet

2 participants