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

No disconnecting when component unmounts. #3

Open
tomisu opened this issue Sep 17, 2019 · 7 comments
Open

No disconnecting when component unmounts. #3

tomisu opened this issue Sep 17, 2019 · 7 comments

Comments

@tomisu
Copy link
Contributor

tomisu commented Sep 17, 2019

While using this, I found this problem: if the component using this hook unmounts, React complains about a memory leak.

To fix this, resizeObserver should disconnect on useEffect's return function:

export const useResizeObserver = (ref: RefObject<HTMLElement>, callback: ObserverCallback) => {
  useEffect(() => {
    const resizeObserver = new (window as any).ResizeObserver((entries: ResizeObserverEntry[]) => {
      callback(entries[0].contentRect);
    });

    resizeObserver.observe(ref.current);

    return () => {
      resizeObserver.disonnect();
    }
  }, [ref]);
}
@zzarcon
Copy link
Owner

zzarcon commented Sep 17, 2019

Hi @tomisu ! Thanks for opening this.

What you are saying makes a lot of sense to me, would you like to submit a PR with that change? I will more than happy to merge it :)

@tomisu
Copy link
Contributor Author

tomisu commented Sep 18, 2019

@zzarcon I don't seem to have permission to push into a new branch.ERROR: Permission to zzarcon/react-resize-observer-hook.git denied to tomisu.

@yvt
Copy link

yvt commented Oct 3, 2019

@tomisu You can create a fork and push the change to it. After that, you can create a pull request based on your fork (see GitHub's documentation on PR).

But since you are making a small change, there's an easier way to do this: Go to the file you want to change on GitHub, and click the pencil icon on the upper-right corner. After that, the website should guide you through the process.

@tomisu
Copy link
Contributor Author

tomisu commented Oct 4, 2019

Thanks @yvt ! I didn't know that.

Here's the PR; I hope I did everything OK:
#4

@Lanchez
Copy link

Lanchez commented Nov 5, 2019

Isn't there a typo there? resizeObserver.disonnect(); <-missing c

@tomisu
Copy link
Contributor Author

tomisu commented Nov 6, 2019

You're totally right, how embarrassing!

I crated a PR: #6

Thanks!

@yvt
Copy link

yvt commented Apr 2, 2020

@zzarcon Since these PRs are all merged and it's been a while since then, can you publish a new version to npm?

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

No branches or pull requests

4 participants