Components not cleaned up with turbo links navigation

[mmccall10]

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]

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

[jkbailey]

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

[BookOfGreg]

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.

[mmccall10]

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

[calleluks]

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]

@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.

[olleolleolle]

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

[jdelStrother]

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.