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

Fix turbolinks mount event #764

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Fix turbolinks mount event #764

merged 2 commits into from
Mar 21, 2017

Conversation

szyablitsky
Copy link
Contributor

@szyablitsky szyablitsky commented Mar 19, 2017

This change fixes "The node you're attempting to unmount was rendered by another copy of React." error when navigating to cached page using Turbolinks.

More info can be found at renchap/webpacker-react#14 (comment) and https://sevos.io/2017/02/27/turbolinks-lifecycle-explained.html


This change is Reviewable

@coveralls
Copy link

coveralls commented Mar 19, 2017

Coverage Status

Coverage remained the same at 97.898% when pulling 8ea8647 on szyablitsky:master into bd974c0 on shakacode:master.

@justin808
Copy link
Member

@szyablitsky Thanks! Any details on how you resolved this issue?


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


node_package/src/clientStartup.js, line 201 at r1 (raw file):

      );
    }
    reactOnRailsPageLoaded();

@szyablitsky Are we sure that the event handlers on turbolinks:render or page:change won't causes a double render?


Comments from Reviewable

@szyablitsky
Copy link
Contributor Author

szyablitsky commented Mar 19, 2017

I included a link to the article where this issue explained in full details.
In short, when user visits cached page, Turbolinks first render cached version, then (when page is loaded) render loaded version, so we have two turbolinks:before-render events and one turbolinks:load event. Second turbolinks:before-render tries to unmount components but they are already unmounted. So we use turbolinks:render which is called after each turbolinks:before-render. And we should mount components on page load (DOMContentLoaded), because Turbolinks do not fire turbolinks:render when page is loaded by browser.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


node_package/src/clientStartup.js, line 201 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@szyablitsky Are we sure that the event handlers on turbolinks:render or page:change won't causes a double render?

My bad. Forgot about Turbolinks 2. Will fix now.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Mar 19, 2017

Coverage Status

Coverage remained the same at 97.898% when pulling d987b38 on szyablitsky:master into bd974c0 on shakacode:master.

@justin808
Copy link
Member

Looks good. 100% sure we can ship, @szyablitsky?


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


node_package/src/clientStartup.js, line 201 at r1 (raw file):

Previously, szyablitsky (Sergey Zyablitsky) wrote…

My bad. Forgot about Turbolinks 2. Will fix now.

Let me know when you're sure we're good to go and I'll release this!


Comments from Reviewable

@szyablitsky
Copy link
Contributor Author

I'm sure this is the best option of handling React with Turbolinks 5, which works in all scenarios. We definitely can ship it.


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from Reviewable

@szyablitsky
Copy link
Contributor Author

We need to rebuild on CI because it's timed out on bundle install.

@justin808
Copy link
Member

As soon as I can get this to pass CI, I'll release.

@justin808
Copy link
Member

error:

Installing rails-html-sanitizer 1.0.3
Installing capybara 2.12.1
Gem::RemoteFetcher::UnknownHostError: timed out
(https://rubygems.org/gems/pry-rescue-1.4.5.gem)
An error occurred while installing pry-rescue (1.4.5), and Bundler
cannot continue.
Make sure that `gem install pry-rescue -v '1.4.5'` succeeds before bundling.
The command "bundle install" failed 3 times.
The command "travis_retry bundle install" failed and exited with 5 during .
Your build has been stopped.

@justin808
Copy link
Member

I'll restart

@justin808 justin808 merged commit 081afdf into shakacode:master Mar 21, 2017
@justin808
Copy link
Member

Thanks @szyablitsky

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

Successfully merging this pull request may close these issues.

3 participants