Using turbolinks:before-visit causes too early unmount and components "flickering" #706

Closed
szyablitsky opened this Issue Feb 4, 2017 · 24 comments

Comments

Projects
None yet
3 participants
@szyablitsky
Contributor

szyablitsky commented Feb 4, 2017

Changing unmount event to turbolinks:before-render causes warning. See #610

unmountComponentAtNode(): The node you're attempting to unmount was rendered by another copy of React.

If ReactDOM.unmountComponentAtNode can leave rendered markup after component unmounting, then we can use turbolinks:before-visit and get rid of "flickering" components causing bad UX.

I filled a pull request to React adding shouldLeaveMarkup flag for unmountComponentAtNode
facebook/react#8928

Tested these changes on my local copies of libraries and they resolve this issue.

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 6, 2017

Contributor

@justin808, @volkanunsal
I get an answer about my PR from Dan Abramov. He rightly asked why we get an error about 'another copy of React' in first place. I locally changed hook back to turbolinks:before-render and I can not reproduce 'another copy of React' warning by any means. The only way I was able to get this warning is to do a modification in component which leads to an error while rendering component when state was changed wrong way.
But I get this warning after rendering error either way with turbolinks:before-render and turbolinks:before-visit

Can anyone provide me with exact scenario how to get this warning only in case of turbolinks:before-render?

I propose to change event back to turbolinks:before-render or provide a configuration option for event used for unmount.

Related issues: #610, #629

Contributor

szyablitsky commented Feb 6, 2017

@justin808, @volkanunsal
I get an answer about my PR from Dan Abramov. He rightly asked why we get an error about 'another copy of React' in first place. I locally changed hook back to turbolinks:before-render and I can not reproduce 'another copy of React' warning by any means. The only way I was able to get this warning is to do a modification in component which leads to an error while rendering component when state was changed wrong way.
But I get this warning after rendering error either way with turbolinks:before-render and turbolinks:before-visit

Can anyone provide me with exact scenario how to get this warning only in case of turbolinks:before-render?

I propose to change event back to turbolinks:before-render or provide a configuration option for event used for unmount.

Related issues: #610, #629

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 6, 2017

Contributor
Contributor

volkanunsal commented Feb 6, 2017

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 6, 2017

Contributor

You are wrong in your confidence that turbolinks replaces javascript code on every page load. I can access registered ReactOnRails store even when I navigate via turbolinks ten pages away just by calling ReactOnRails.getStore('SomePreviouslyPopulatedStore') from console or in code on other pages. It stays in memory despite any turbolinks visits.
I even placed the check inside https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js to prove that we have the same copy of React between turbolinks visits.
It saves ReactDOM on render and compares it (strict equality ===) with ReactDOM on which we call unmount when the page is unloaded. And it is the same all the times.

Contributor

szyablitsky commented Feb 6, 2017

You are wrong in your confidence that turbolinks replaces javascript code on every page load. I can access registered ReactOnRails store even when I navigate via turbolinks ten pages away just by calling ReactOnRails.getStore('SomePreviouslyPopulatedStore') from console or in code on other pages. It stays in memory despite any turbolinks visits.
I even placed the check inside https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js to prove that we have the same copy of React between turbolinks visits.
It saves ReactDOM on render and compares it (strict equality ===) with ReactDOM on which we call unmount when the page is unloaded. And it is the same all the times.

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 6, 2017

Contributor
Contributor

volkanunsal commented Feb 6, 2017

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 7, 2017

Member

@volkanunsal @szyablitsky I'd really appreciate it if you two can work together to come up with the best recommendation for our community. Some thoughts:

  1. Turbolinks is not compatible with React-Router. Eventually, I'd like to fork turbolinks to make a version that allows using React-Router...
  2. Should we consider not supporting turbolinks-classic going forward? What is the maintenance burden of supporting both?
  3. How can we support dynamic code splitting with TurboLinks (or a derivation of it)? What about features like "shared redux stores"?

CC: @robwise, @alexfedoseev

Member

justin808 commented Feb 7, 2017

@volkanunsal @szyablitsky I'd really appreciate it if you two can work together to come up with the best recommendation for our community. Some thoughts:

  1. Turbolinks is not compatible with React-Router. Eventually, I'd like to fork turbolinks to make a version that allows using React-Router...
  2. Should we consider not supporting turbolinks-classic going forward? What is the maintenance burden of supporting both?
  3. How can we support dynamic code splitting with TurboLinks (or a derivation of it)? What about features like "shared redux stores"?

CC: @robwise, @alexfedoseev

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 7, 2017

Contributor

@justin808 I did not want to use ReactRouter before it reaches major version 4, so I wrote my own lightweight router (~100 LOC), which is buggy in some places (back navigation), but I already have a plan how to fix it. Also I'm going to look at https://junctions.js.org/
Forking Turbolinks seems to be an acceptable idea. But I'd say a full rewrite in plain JavaScript (ES6) would be preferable. I always have a very hard time when I have to mentally parse CoffeeScript.

Supporting both versions of turbolinks adds 5 LOC, so it is not a concern. It can be, but only in case of forking Turbolinks and changing event names.

As of "shared redux stores" if you mean using one store by many independent components via ReactnRails.getStore - I didn't have any issues with them and Turbolinks.
One little thing. We can clean up registered stores when we unmount component on page unload. So they won't be available on next pages.

@volkanunsal you mentioned some HelloWorld application in #610. Is it your own project or Dummy from this project or https://github.com/shakacode/react-webpack-rails-tutorial? Can you share it? I'd like to reproduce this error and try to fix it. Dan Abramov closed my PR, so we have to find another solution.

Contributor

szyablitsky commented Feb 7, 2017

@justin808 I did not want to use ReactRouter before it reaches major version 4, so I wrote my own lightweight router (~100 LOC), which is buggy in some places (back navigation), but I already have a plan how to fix it. Also I'm going to look at https://junctions.js.org/
Forking Turbolinks seems to be an acceptable idea. But I'd say a full rewrite in plain JavaScript (ES6) would be preferable. I always have a very hard time when I have to mentally parse CoffeeScript.

Supporting both versions of turbolinks adds 5 LOC, so it is not a concern. It can be, but only in case of forking Turbolinks and changing event names.

As of "shared redux stores" if you mean using one store by many independent components via ReactnRails.getStore - I didn't have any issues with them and Turbolinks.
One little thing. We can clean up registered stores when we unmount component on page unload. So they won't be available on next pages.

@volkanunsal you mentioned some HelloWorld application in #610. Is it your own project or Dummy from this project or https://github.com/shakacode/react-webpack-rails-tutorial? Can you share it? I'd like to reproduce this error and try to fix it. Dan Abramov closed my PR, so we have to find another solution.

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 7, 2017

Member

@justin808 I did not want to use ReactRouter before it reaches major version 4, so I wrote my own lightweight router (~100 LOC), which is buggy in some places (back navigation), but I already have a plan how to fix it. Also I'm going to look at https://junctions.js.org/
Forking Turbolinks seems to be an acceptable idea. But I'd say a full rewrite in plain JavaScript (ES6) would be preferable. I always have a very hard time when I have to mentally parse CoffeeScript.

ReactRouter v4 is great! We're using it already at egghead.io

Supporting both versions of turbolinks adds 5 LOC, so it is not a concern. It can be, but only in case of forking Turbolinks and changing event names.
OK

As of "shared redux stores" if you mean using one store by many independent components via ReactnRails.getStore - I didn't have any issues with them and Turbolinks.
One little thing. We can clean up registered stores when we unmount component on page unload. So they won't be available on next pages.

With React router, it's understood that one needs to fetch the data as the page changes...

We'd probably want the same behavior for shared stores since we're not re-rendering the page.

@volkanunsal you mentioned some HelloWorld application in #610. Is it your own project or Dummy from this project or https://github.com/shakacode/react-webpack-rails-tutorial? Can you share it? I'd like to reproduce this error and try to fix it. Dan Abramov closed my PR, so we have to find another solution.

I'm not sure. Maybe the default generator plus turbolinks turned on? Maybe the /spec/dummy app. Maybe the https://github.com/shakacode/react-webpack-rails-tutorial.

Member

justin808 commented Feb 7, 2017

@justin808 I did not want to use ReactRouter before it reaches major version 4, so I wrote my own lightweight router (~100 LOC), which is buggy in some places (back navigation), but I already have a plan how to fix it. Also I'm going to look at https://junctions.js.org/
Forking Turbolinks seems to be an acceptable idea. But I'd say a full rewrite in plain JavaScript (ES6) would be preferable. I always have a very hard time when I have to mentally parse CoffeeScript.

ReactRouter v4 is great! We're using it already at egghead.io

Supporting both versions of turbolinks adds 5 LOC, so it is not a concern. It can be, but only in case of forking Turbolinks and changing event names.
OK

As of "shared redux stores" if you mean using one store by many independent components via ReactnRails.getStore - I didn't have any issues with them and Turbolinks.
One little thing. We can clean up registered stores when we unmount component on page unload. So they won't be available on next pages.

With React router, it's understood that one needs to fetch the data as the page changes...

We'd probably want the same behavior for shared stores since we're not re-rendering the page.

@volkanunsal you mentioned some HelloWorld application in #610. Is it your own project or Dummy from this project or https://github.com/shakacode/react-webpack-rails-tutorial? Can you share it? I'd like to reproduce this error and try to fix it. Dan Abramov closed my PR, so we have to find another solution.

I'm not sure. Maybe the default generator plus turbolinks turned on? Maybe the /spec/dummy app. Maybe the https://github.com/shakacode/react-webpack-rails-tutorial.

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 7, 2017

Contributor

If you want to reuse shared store between page visits, you have to consider that data on the server could be changed already. There must be a mechanism that keeps the data fresh in "realtime".

Contributor

szyablitsky commented Feb 7, 2017

If you want to reuse shared store between page visits, you have to consider that data on the server could be changed already. There must be a mechanism that keeps the data fresh in "realtime".

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 7, 2017

Contributor
Contributor

volkanunsal commented Feb 7, 2017

@szyablitsky szyablitsky changed the title from Using turbolinks:before-visit causes to early unmount and components "flickering" to Using turbolinks:before-visit causes too early unmount and components "flickering" Feb 7, 2017

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 8, 2017

Member

@volkanunsal, are you using TurboLinks? Maybe I can create a PR that could do something like allow it to "know" that some URLs are handled by react-router?

@szyablitsky In terms of page navigation, if the route is handled by rails, then we should consider invoking the store initialization with "new" data for a different page that Rails is rendering...I think any logic can be in the generator function for the custom redux store.

Let's figure out the best course of action here! If any of you want slack invites to the ShakaCode slack, email me.

Member

justin808 commented Feb 8, 2017

@volkanunsal, are you using TurboLinks? Maybe I can create a PR that could do something like allow it to "know" that some URLs are handled by react-router?

@szyablitsky In terms of page navigation, if the route is handled by rails, then we should consider invoking the store initialization with "new" data for a different page that Rails is rendering...I think any logic can be in the generator function for the custom redux store.

Let's figure out the best course of action here! If any of you want slack invites to the ShakaCode slack, email me.

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 8, 2017

Contributor

I checked /spec/dummy in master branch. Changed Turbolinks event to turboliks:before-render. The only page that gave me another copy of React warning was /server_side_log_throw, which simulates rendering error. All other pages are working without warnings, navigating in either direction back and forth.

I changed binding event back to turboliks:before-visit. The result are the same. No warnings except /server_side_log_throw page.

It clearly demonstrates IMO, that Turbolinks event and another copy of React warning are not related together.

I even added a code that saved ReactDOM reference when components are rendering and assert just before unmounting. Assert is comparing saved reference with current ReactDOM. The assert did not throw at all. Even on /server_side_log_throw page when we get another copy of React warning.

If no one is mind, I will submit the PR changing unmount event back to turboliks:before-render.

Contributor

szyablitsky commented Feb 8, 2017

I checked /spec/dummy in master branch. Changed Turbolinks event to turboliks:before-render. The only page that gave me another copy of React warning was /server_side_log_throw, which simulates rendering error. All other pages are working without warnings, navigating in either direction back and forth.

I changed binding event back to turboliks:before-visit. The result are the same. No warnings except /server_side_log_throw page.

It clearly demonstrates IMO, that Turbolinks event and another copy of React warning are not related together.

I even added a code that saved ReactDOM reference when components are rendering and assert just before unmounting. Assert is comparing saved reference with current ReactDOM. The assert did not throw at all. Even on /server_side_log_throw page when we get another copy of React warning.

If no one is mind, I will submit the PR changing unmount event back to turboliks:before-render.

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 8, 2017

Member

Thanks for the PR @szyablitsky. I really need you and @volkanunsal to give me a clear articulation of the tradeoffs, and whether or not this should be configurable. As I'm not using TurboLinks right now, I have no idea which is better. We should also update this file: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/turbolinks.md

Member

justin808 commented Feb 8, 2017

Thanks for the PR @szyablitsky. I really need you and @volkanunsal to give me a clear articulation of the tradeoffs, and whether or not this should be configurable. As I'm not using TurboLinks right now, I have no idea which is better. We should also update this file: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/turbolinks.md

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 8, 2017

Contributor

@justin808 I fixed turbolinks.md. I do not think that unmount event should be configurable. And I do not see any tradeoffs of using turbolinks:before-render. I'm even sure now that we followed wrong assumptions when changed event to before-visit (before-cache).

@volkanunsal do you use Turbolinks with ReactOnRails in any of your current projects? Can you try the same procedure as I described to check if I'm right or wrong?

Contributor

szyablitsky commented Feb 8, 2017

@justin808 I fixed turbolinks.md. I do not think that unmount event should be configurable. And I do not see any tradeoffs of using turbolinks:before-render. I'm even sure now that we followed wrong assumptions when changed event to before-visit (before-cache).

@volkanunsal do you use Turbolinks with ReactOnRails in any of your current projects? Can you try the same procedure as I described to check if I'm right or wrong?

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 8, 2017

Contributor
Contributor

volkanunsal commented Feb 8, 2017

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 8, 2017

Contributor
Contributor

volkanunsal commented Feb 8, 2017

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 8, 2017

Member

@volkanunsal: I invited you to the ShakaCode Slack.

@szyablitsky: Let's find a time to screen share and go over this, maybe with Volkan. I'm quite sure that the 3 of us can nail this issue.

Member

justin808 commented Feb 8, 2017

@volkanunsal: I invited you to the ShakaCode Slack.

@szyablitsky: Let's find a time to screen share and go over this, maybe with Volkan. I'm quite sure that the 3 of us can nail this issue.

@volkanunsal

This comment has been minimized.

Show comment
Hide comment
@volkanunsal

volkanunsal Feb 9, 2017

Contributor

@szyablitsky @justin808 I just tested the before-render event hook in one of my apps. It seems to work without errors. I may have been wrong about before-render. Either I didn't test it correctly in the discussion thread of #629 or something changed in React. I don't know. Either way, I would be ok with using that event ok without configuration options as per @szyablitsky's recommendation.

Contributor

volkanunsal commented Feb 9, 2017

@szyablitsky @justin808 I just tested the before-render event hook in one of my apps. It seems to work without errors. I may have been wrong about before-render. Either I didn't test it correctly in the discussion thread of #629 or something changed in React. I don't know. Either way, I would be ok with using that event ok without configuration options as per @szyablitsky's recommendation.

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Feb 9, 2017

Member

See #709 Do we have consensus?

Member

justin808 commented Feb 9, 2017

See #709 Do we have consensus?

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 9, 2017

Contributor

I think I will add an option turbolinksUnmountOnBeforeRender default false. After release I will test it in production with turbolinksUnmountOnBeforeRender: true. And if it will be working stable, we'll remove his option and bind unmount event to turbolinks:before-render. Or we can switch option to turbolinksUnmountOnBeforeVisit deafult false.

Contributor

szyablitsky commented Feb 9, 2017

I think I will add an option turbolinksUnmountOnBeforeRender default false. After release I will test it in production with turbolinksUnmountOnBeforeRender: true. And if it will be working stable, we'll remove his option and bind unmount event to turbolinks:before-render. Or we can switch option to turbolinksUnmountOnBeforeVisit deafult false.

@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 9, 2017

Contributor

Added an option. Updated PR.

Contributor

szyablitsky commented Feb 9, 2017

Added an option. Updated PR.

@justin808 justin808 closed this in #709 Feb 11, 2017

justin808 added a commit that referenced this issue Feb 11, 2017

Change Turbolinks unmount event from before-visit to before-render (#709
)

* change turbolinks unmount event from before-visit to before-render
* Addresses #706

justin808 added a commit that referenced this issue Feb 11, 2017

Change Turbolinks unmount event from before-visit to before-render (#709
)

* change turbolinks unmount event from before-visit to before-render
* Addresses #706
@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Feb 26, 2017

Contributor

renchap/webpacker-react#14 (comment)
It seems that real problem comes from mount event (not unmount).
I will try this solution and submit а PR.

Contributor

szyablitsky commented Feb 26, 2017

renchap/webpacker-react#14 (comment)
It seems that real problem comes from mount event (not unmount).
I will try this solution and submit а PR.

@justin808 justin808 reopened this Mar 5, 2017

@justin808

This comment has been minimized.

Show comment
Hide comment
@justin808

justin808 Mar 5, 2017

Member

@szyablitsky Thanks for looking into this!

Member

justin808 commented Mar 5, 2017

@szyablitsky Thanks for looking into this!

@szyablitsky szyablitsky referenced this issue in reactjs/react-rails Mar 15, 2017

Closed

turbolinks:before-render for unmounting causes warnings #607

1 of 4 tasks complete
@szyablitsky

This comment has been minimized.

Show comment
Hide comment
@szyablitsky

szyablitsky Mar 19, 2017

Contributor

I reproduced The node you're attempting to unmount was rendered by another copy of React. error. It happens after navigation twice or more times to the same page. I changed mount event to DOMContentLoaded and turbolinks:render as described in @sevos article and it removes the error.

PR #764

Contributor

szyablitsky commented Mar 19, 2017

I reproduced The node you're attempting to unmount was rendered by another copy of React. error. It happens after navigation twice or more times to the same page. I changed mount event to DOMContentLoaded and turbolinks:render as described in @sevos article and it removes the error.

PR #764

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