kobaltedev / kobalte

A UI toolkit for building accessible web apps and design systems with SolidJS.
https://kobalte.dev
MIT License
1.2k stars 61 forks source link

Better ResizeObserver hook #434

Closed thednp closed 1 month ago

thednp commented 1 month ago

I noticed some performance issues, probably some memory leaks related, and I noticed you're using an interesting ResizeObserver implementation, which I think it's not good enough because you're looping through all nodes and it's an expensive operation. Also you're missing out other rect attributes such as x, y, top, bottom, etc.

Here's an example of a potentially better implementation (inspired by useIntersectionObserver hook from React):

import { createEffect, onCleanup, createSignal } from 'solid-js';

export function useResizeObserver() {
  const [entry, setEntry] = createSignal<ResizeObserverEntry>();
  let previousObserver: ResizeObserver;

  const customRef = <T extends Element>(node: T) => {
    if (previousObserver) {
      previousObserver.disconnect();
    }
    if (node?.nodeType === Node.ELEMENT_NODE) {
      previousObserver = new ResizeObserver(
        ([entry]) => {
          setEntry(entry);
        },
      );
      previousObserver.observe(node);
    }
  };

  createEffect(() => {
    if (previousObserver) {
      onCleanup(() => previousObserver.disconnect());
    }
  });

  return [customRef, entry] as [typeof customRef, typeof entry];
}

Usage

export default function Home() {
  const [resizeRef, observerEntry] = useResizeObserver();

  createEffect(() => {
    console.log(observerEntry());
  })

  return (
      <main ref={resizeRef} class="w-full h-full">
        Demo content
     </main>
  )
}

A similar implementation can be done with IntersectionObserver.

Please let me know what you think.

jer3m01 commented 1 month ago

cc @GiyoMoon

GiyoMoon commented 1 month ago

Good call about the loop. I didn't fully understand how the entries work and that the loop isn't required as there always will be a single entry in this use case. I'll fix this.

But why do you think that this would cause memory leaks? It's a single loop, looping over an array with one entry.

thednp commented 1 month ago

I wish I could explain like I was Ryan, but I think you should test the difference yourself, basically you're adding additional scopes, variables and additional effects for just 2 values, instead of getting all values with minimal work.

I don't know exactly how to explain memory leaks, but I know for sure that poor performance especially around resize translates to bad memory management. May not seem like a big problem, but still I have a pretty powerful PC and janky resize response caught my eye.

The way hooks should work generally is to attach logic to DOM nodes like they own them, native, the customRef callback is a genius spark of power.

Let me know what you think.

GiyoMoon commented 1 month ago

for just 2 values, instead of getting all values with minimal work.

Well, we only need these two values in our use case. No need to keep the whole entry in memory if we don't need it. And imo it perfectly makes sense to use an effect here, as we're working with the DOM and need to cleanup after us. Like you're using one too to disconnect the observer. I don't see how our implementation would cause any noticeable performance issues. Usually, laggy resizing comes from the browser recalculating many elements.

thednp commented 1 month ago

I've been working on an example to showcase the problem but createSize just won't play nice, it will crash the browser in a split second. So I'm pasting here an updated useResizeObserver version with configurable debounce and onResize callback.

import { onCleanup, createSignal } from 'solid-js';

type Box = { width: number; height: number };
type ResizeHookProps = { timeout: number, onResize?: (box: Box) => void };

export function useResizeObserver({ timeout, onResize }: ResizeHookProps = { timeout: 1 }) {
    const [entry, setEntry] = createSignal<Box>();
    let previousObserver: ResizeObserver | undefined;
    let timer: ReturnType<typeof setTimeout> | undefined;

    const customRef = <T extends HTMLElement>(node: T) => {
        if (typeof window === 'undefined' || !('ResizeObserver' in window)) return;

        if (node?.nodeType === Node.ELEMENT_NODE) {
            previousObserver = new ResizeObserver(
                () => {
                    clearTimeout(timer);
                    timer = setTimeout(() => {
                        const width = node.offsetWidth;
                        const height = node.offsetHeight;
                        setEntry({ width, height });
                        if (typeof onResize === 'function') onResize({ width, height });
                    }, timeout);
                }
            );
            previousObserver.observe(node);
        }
    };

    onCleanup(() => {
        if (previousObserver) {
            previousObserver.disconnect();
            previousObserver = undefined;
        }
    });

    return [customRef, entry] as [typeof customRef, typeof entry];
}

In my tests, this will never crash, jank or do anything to the resize of a window.

Let me know if you want to see the example I've been working on.

GiyoMoon commented 1 month ago

Yes a reproduction would be very helpful :) Thanks

thednp commented 1 month ago

@GiyoMoon as requested: solid-resize.zip

GiyoMoon commented 1 month ago

Hey @thednp, I'm getting an error on the second page:

image
thednp commented 1 month ago

@GiyoMoon Yea, I cannot find a way to initialize your script. Please try and make it work the way you designed it and compare the two if you can.

GiyoMoon commented 1 month ago

@thednp Please provide a working reproduction with a step-by-step description on how to reproduce the performance issues you're facing with createSize. I'm happy to help but I need something to work with.

thednp commented 1 month ago

This component is using your createSize and it feels janky when resizing the window. First time I noticed it I looked into the code and I saw it's using your implementation and I wanted to say it's not the best.

But anyways, feel free to close this if you don't care or it bothers you. The code I provided was reworked over and over in a couple of hours to get the best possible outcome.

GiyoMoon commented 1 month ago

Thanks. I briefly looked at the performance tab and it doesn't seem like the createSize code is causing any meaningful lag. Most of the blocking comes from animating or repainting the UI. The Navigation Menu has transition-property: width,height; applied. Maybe this could lead to it feeling janky? Floating UI is also doing a lot of stuff when resizing it seems. It's responsible for all the resize events that have to be handled. The resize observer doesn't fire nearly as often here. image

jer3m01 commented 1 month ago

Closing as this doesn't seem to be the cause of performance issues. Happy to reopen if we find benchmarks backing it.