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 #1028

Closed
mmccall10 opened this issue Sep 26, 2019 · 8 comments
Closed

Components not cleaned up with turbo links navigation #1028

mmccall10 opened this issue Sep 26, 2019 · 8 comments
Labels
bug Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted

Comments

@mmccall10
Copy link

Steps to reproduce

Navigate with turbolinks to pages with render_component.

Expected behavior

Components should be unmounted when navigating to different pages.

Actual behavior

Each page visit renders a new component while leaving the previous ones in place. With turbolinks cached pages subsequent page visits renders 2 new components from what I assume is the cached page, then the new page content.

System configuration

Sprockets or Webpacker version: webpacker (4.0.7)
React-Rails version: react-rails (2.6.0)
Rect_UJS version: react_ujs@^2.6.0
Rails version: 6.0
Ruby version: 2.6.3


It looks like a fix for this might have been submitted with PR #1020. That is almost a month old so maybe its a different issue?

Adding the following code to application.js seems to work as a temporary work around.

document.addEventListener('turbolinks:before-render', () => {
    ReactRailsUJS.handleUnmount()
});

It appears the default way ReactRailsUjs is handling unmounting with turbolinks is the issue.

Sample app that recreates the problem.
https://github.com/mmccall10/turbolinks-react-rails-demo

@marcelokanzaki
Copy link

I can confirm that. @mmccall10 thanks for the quick fix. Let's wait for the PR now...

@jkbailey
Copy link
Contributor

Thanks for mentioning my PR, also thanks for the quick fix for now. Not sure how to speed up the PR though.

@BookOfGreg
Copy link
Member

The problem I'm having with PRs at the moment is mostly around the general JS mess. Since Webpack split out Webpack-cli in a teeny patch, not following semantic versioning at all, the pipeline has been broken. Also Chrome stopped supporting the normal selenium API in the last release so I've been spending vast majority of my time just trying to get master built.

I recognize this is an issue for you and the linked PR is valid and correct, sorry this is taking so long. I'm really frustrated too.

@BookOfGreg BookOfGreg added bug Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted labels Oct 2, 2019
@mmccall10
Copy link
Author

mmccall10 commented Oct 2, 2019

@BookOfGreg thanks for the response. No pressure! Also, thanks for confirming.

@calleluks
Copy link
Contributor

Hey @mmccall10 and @BookOfGreg, I'm probably missing something here so please excuse what might be a dumb question:

When navigating with Turbolinks, why do we need to unmount the components?

Turbolinks uses cloneNode(true) when caching pages so we shouldn't risk leaving old event listeners around.

I'm working on an app using both RailsReactUJS and Turbolinks that has issues restoring the scroll position when navigating back in the browser history. One page has a really tall React component. If you scroll down the page, click a link, and click back, Turbolinks fails to restore the scroll position because at the time it tries to do so, the component has yet to be mounted, leaving the page mostly empty.

When I remove the turbolinks:before-render event listener that unmounts the component it works fine:

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

What I don't understand is what problems this may cause. When I click between the pages in the demo app linked to below, I can't see any issues. What am I missing?

Doing something like this (instead of the current Turbolinks integration) to mount components before Turbolinks restores the scroll position also seems to work:

ReactRailsUJS.handleEvent('turbolinks:load', ReactRailsUJS.handleMount)

ReactRailsUJS.handleEvent('turbolinks:before-render', function (e) {
  ReactRailsUJS.removeEvent('turbolinks:load', ReactRailsUJS.handleMount)

  ReactRailsUJS.unmountComponents(document.body)
  ReactRailsUJS.mountComponents(e.data.newBody)
})

@BookOfGreg
Copy link
Member

@calleluks Generally the JS community likes to break stuff. Back when this was an issue, Turbolinks + React kept rendering multiple copies of React components when Turbolinks hooks happened.
It could be the case that React is smart enough to not double-render, or that Turbolinks is smart enough to unmount things on it's own.
Times change, maybe it's time to undo this to support newest versions of Turbolinks or React? Would welcome some experimentation + a PR.

calleluks added a commit to calleluks/react-rails that referenced this issue Jun 24, 2021
[As discussed in reactjs#1028](reactjs#1028 (comment)), unmounting components on `turbolinks:before-render` prevents Turbolinks from restoring the scroll position when navigating back to pages with tall React components. This happens because Turbolinks tries to restore the scroll position before firing the `turbolinks:load` event. Since components were unmounted prior to this, the page might be much shorter than it will be after mounting components causing scroll position restoration to fail.

We've been running this in production for a week now and haven't noticed any issues caused by not unmounting components.
@olleolleolle
Copy link

Ah, cool, there's a PR now! Is #1135 mergeable?

@jdelStrother
Copy link

So now that that PR's been merged, 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.

The PR talks about fixing the scroll-position restoration, but that seems like something that should have been fixed upstream in Turbolinks rather than removing component-unmounts from ReactUJS.

HardCarrier pushed a commit to HardCarrier/react-rails that referenced this issue Jan 9, 2023
[As discussed in #1028](reactjs/react-rails#1028 (comment)), unmounting components on `turbolinks:before-render` prevents Turbolinks from restoring the scroll position when navigating back to pages with tall React components. This happens because Turbolinks tries to restore the scroll position before firing the `turbolinks:load` event. Since components were unmounted prior to this, the page might be much shorter than it will be after mounting components causing scroll position restoration to fail.

We've been running this in production for a week now and haven't noticed any issues caused by not unmounting components.
goldapple911 added a commit to goldapple911/react-rails that referenced this issue Aug 15, 2023
[As discussed in #1028](reactjs/react-rails#1028 (comment)), unmounting components on `turbolinks:before-render` prevents Turbolinks from restoring the scroll position when navigating back to pages with tall React components. This happens because Turbolinks tries to restore the scroll position before firing the `turbolinks:load` event. Since components were unmounted prior to this, the page might be much shorter than it will be after mounting components causing scroll position restoration to fail.

We've been running this in production for a week now and haven't noticed any issues caused by not unmounting components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted
Projects
None yet
Development

No branches or pull requests

7 participants