Components aren't unmounted when Turbolinks cache is disabled

[volkanunsal]

This issue is related to #610. Listening to turbolinks:before-cache event results in component not being unmounted when the cache is disabled. I propose using turbolinks:before-visit event in addition to it, which would cover all the bases. (However, if both events are fired off consecutively, this approach might cause problems. It's worth investigating further.)

[justin808]

@volkanunsal Please keep researching this. Eventually, we might want to create a customized offshoot of turbolinks that can work with react-router and code-splitting by routes.

[szyablitsky]

#425 fixes for me the 'flickering' of component which is rendered on every page when navigating between pages.
So changing turbolinks:before-render to turbolinks:before-cache fixes one problem but has a regression on the other.

[justin808]

@szyablitsky @volkanunsal The community will greatly appreciate any research into this matter, and a PR!

[volkanunsal]

@justin808 I've been meaning to come back to this. I'm going to look into it tomorrow!

[volkanunsal]

I discovered that turbolinks:before-visit is called even earlier than turbolinks:before-cache, and it's never omitted like turbolinks:before-cache is, even when cache is disabled. So It can easily be used to cover all cases. I'm going to submit a PR replacing turbolinks:before-cache with turbolinks:before-visit, unless there are concerns about backward compatibility.

[szyablitsky]

Maybe we should make this configurable?
In our project we use turbolinks:before-render to minimize the absence time for the component which is rendered on every page of the application (#425)

[volkanunsal]

@szyablitsky What version of react are you using? In 15.2.x I get the error discussed in #610

[szyablitsky]

Right now we're using react@15.4.1 & react-on-rails@6.2.0
I've seen this error before, and did not know this can be related to turbolinks. I do not see this error in our application anymore.
Ha-ha. As it turns out react-on-rails@6.2.0 already have merged using turbolinks:before-cache, despite changelog, which says it was merged only in 6.2.1. That's why we do not have "The node you're attempting to unmount was rendered by another copy of React" problem. But we have 'flickering' components problem again.

[volkanunsal]

Disable Turbolinks cache with a meta tag. That should take care of it.

…

On Sun, Dec 11, 2016 at 11:05 PM Sergey Zyablitsky ***@***.***> wrote: Right now we're using ***@***.*** & ***@***.*** I've seen this error before, and did not know this can be related to turbolinks. I do not see this error in our application anymore. Ha-ha. As it turns out ***@***.*** already have merged using turbolinks:before-cache, despite changelog, which says it was merged only in 6.2.1. That's why we do not have "The node you're attempting to unmount was rendered by another copy of React" problem. But we have 'flickering' components problem again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#629 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJQMFkGPnvjSEyMwNuQUOMmqJ6nY4IPks5rHMfxgaJpZM4LAPSY> .

[volkanunsal]

@szyablitsky I mean disable the cache after the latest PR lands. You shouldn't disable it when still using turbolinks:before-cache event hook.

[szyablitsky]

@volkanunsal disabling turbolinks cache doesn't solve "flickering" components problem.
React-rails gem has the same issue reactjs/react-rails#607 and they also don't know how to handle it.

[volkanunsal]

@szyablitsky A possible solution is to use fake content at the mount point. It would look something like the component, and have the same dimensions.

[szyablitsky]

@volkanunsal thank you for the idea. This can be used as a partial solution.

[szyablitsky]

I think the best solution is to modify ReactDOM's unmountComponentAtNode and more specifically unmountComponentFromNode so it can bypass emptying of the container https://github.com/facebook/react/blob/8791325db03725ef4801fc58b35a3bb4486a8904/src/renderers/dom/stack/client/ReactMount.js#L190
May be additional boolean parameter like doNotEmptyContainer. And use this parameter in our Turbolinks handlers.