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

Components not cleaned up with turbo links navigation, part 2. #1184

Open
jdelStrother opened this issue Jun 14, 2022 · 6 comments
Open

Components not cleaned up with turbo links navigation, part 2. #1184

jdelStrother opened this issue Jun 14, 2022 · 6 comments

Comments

@jdelStrother
Copy link

jdelStrother commented Jun 14, 2022

(I'd originally mentioned this in #1028, but since that's closed I wanted to open an issue for it to get some extra visibility)

Steps to reproduce

  • Write a component that performs cleanup in componentWillUnmount
  • visit a page containing that component
  • navigate away with turbolinks

Expected behavior

componentWillUnmount is called

Actual behavior

componentWillUnmount is never called

System configuration

Sprockets or Webpacker version: 6.4.1
React-Rails version: -
Rect_UJS version: 2.6.2
Rails version: 7.0.2
Ruby version: 3.1


What's the expected way of cleaning up react components on leaving the page?

Some of our components use setInterval to do something every x seconds, which call clearInterval in componentWillUnmount. Others might load and/or decode a large file, which gets aborted in componentWillUnmount.

I could possibly clean these up by having each component listen for turbolinks cleanup events, but having those components have to know that they're living in a turbolinks+react_ujs stack seems like unnecessary coupling.

I think this used to work, but PR1135 removed cleanup in favour of fixing the scroll-position restoration. That seems like something that should have been fixed upstream in Turbolinks rather than removing component-unmounts from ReactUJS ..?

@toreyhickman
Copy link

toreyhickman commented Nov 18, 2022

Just ran into this as well when upgrading from Version 2.6.1 to Version 2.6.2. In our case we have components on some pages that add event listeners when mounted and then remove the event listeners when unmounted. Without the components being unmounted, the listeners are never removed, rather they are added again and again as users navigate through the site.

To correct this, I undid the changes made in #1135 by adding ...

ReactRailsUJS.handleEvent('turbolinks:before-render', ReactRailsUJS.handleUnmount)

... to app/javascript/packs/application.js.

Is there a better way to handle this?

@lcmen
Copy link

lcmen commented Nov 30, 2022

@toreyhickman does this work for you with React 18?

I'm getting a following warning on the console and components are not being unloaded:

react_devtools_backend.js:4026 Warning: You are calling ReactDOM.unmountComponentAtNode() on a container that was previously passed to ReactDOMClient.createRoot(). This is not supported. Did you mean to call root.unmount()?
react_devtools_backend.js:4026 Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by React and is not a top-level container. Instead, have the parent component update its state and rerender in order to remove this component.

@toreyhickman
Copy link

@toreyhickman does this work for you with React 18?

I'm getting a following warning on the console and components are not being unloaded:

react_devtools_backend.js:4026 Warning: You are calling ReactDOM.unmountComponentAtNode() on a container that was previously passed to ReactDOMClient.createRoot(). This is not supported. Did you mean to call root.unmount()?
react_devtools_backend.js:4026 Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by React and is not a top-level container. Instead, have the parent component update its state and rerender in order to remove this component.

@lcmen: The project I'm on is using React 17. I don't know if something in Version 18 would lead to that warning, but I'm not seeing the warning with Version 17.

@lcmen
Copy link

lcmen commented Dec 14, 2022

@toreyhickman I believe React 17 is not affected by this issue just 18 is.

@kylemellander
Copy link
Contributor

This is definitely an issue. While manually adding the event listener to unmount the components "works" it is not ideal for multiple reasons.

  1. You have scenarios like this
  2. Unchanged or data-turbolinks-permanent components unmount unnecessarily

I wonder if there should be some logic that compares the new DOM to the old one to see if the element exists in the new DOM and unmount if it doesn't.

@justin808
Copy link
Collaborator

I'd be happy to consider a PR to fix this. @toreyhickman @lcmen @kylemellander. Let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants