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

Wrong uasge with useLayoutEffect and addEventListener/removeEventListener. #4858

Closed
4 tasks done
wenlixun opened this issue Dec 18, 2023 · 2 comments
Closed
4 tasks done
Labels
bug Something isn't working Liveness

Comments

@wenlixun
Copy link

Before creating a new issue, please confirm:

On which framework/platform are you having an issue?

React

Which UI component?

Liveness

How is your app built?

Webpack

What browsers are you seeing the problem on?

No response

Which region are you seeing the problem in?

No response

Please describe your bug.

Cannot remove the EventListener like below.It may cause the memory rise

What's the expected behaviour?

That should be

useLayoutEffect(() => {
  function fn(e) {
    sendLandscapeWarning(e.matches);
  }
  
  // Listen for future orientation changes and send warning
  landscapeMediaQuery.addEventListener('change', fn);

   // Remove matchMedia event listener
  return () => {
    landscapeMediaQuery.removeEventListener('change', fn);
  };
}, [])

Help us reproduce the bug!

It's easy to see

Code Snippet

React.useLayoutEffect(() => {
    if (isMobile) {
      // ...
      // Listen for future orientation changes and send warning
      landscapeMediaQuery.addEventListener('change', (e) => {
        sendLandscapeWarning(e.matches);
      });

      // Remove matchMedia event listener
      return () => {
        landscapeMediaQuery.removeEventListener('change', (e) =>
          sendLandscapeWarning(e.matches)
        );
      };
    }
  }, [isMobile, send]);

Console log output

No response

Additional information and screenshots

No response

@github-actions github-actions bot added the pending-triage Issue is pending triage label Dec 18, 2023
@thaddmt
Copy link
Contributor

thaddmt commented Jan 3, 2024

Hello @wenlixun ! Thanks for filing the bug. The intention here is for this event listener to only run when the isMobile flag is true. When testing I have found that it does seem to correct add and remove the event listener on mobile devices and does not do so on non-mobile devices. Is there still a concern here?

@thaddmt thaddmt added pending-response bug Something isn't working and removed pending-triage Issue is pending triage labels Jan 3, 2024
@reesscot
Copy link
Contributor

reesscot commented Feb 7, 2024

Closing out as this is working as expected.

@reesscot reesscot closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Liveness
Projects
None yet
Development

No branches or pull requests

3 participants