Support component mounting and unmounting in Turbolinks and PJAX #14

Merged
merged 15 commits into from Feb 26, 2017

Conversation

Projects
None yet
2 participants
@sevos
Contributor

sevos commented Feb 24, 2017

React components will be mounted and unmounted on events specific for:

  • Turbolinks 5
  • Turbolinks 2.4+
  • old, deprecated Turbolinks
  • PJAX
  • without any partial page loading technology present

This code is copied with modifications from https://github.com/reactjs/react-rails with modifications.

This resolves #13

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 24, 2017

Contributor

TBH: I've tested it only on turbolinks 5 and native.

Contributor

sevos commented Feb 24, 2017

TBH: I've tested it only on turbolinks 5 and native.

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 24, 2017

Contributor

We need some test coverage for the new features

Contributor

sevos commented Feb 24, 2017

We need some test coverage for the new features

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 25, 2017

Contributor

There are still some glitches when visiting page multiple times via turbolinks.

After calling Turbolinks.visit(window.location.href) in the js console more than 1 time, I can see the following warning:

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

sevos commented Feb 25, 2017

There are still some glitches when visiting page multiple times via turbolinks.

After calling Turbolinks.visit(window.location.href) in the js console more than 1 time, I can see the following warning:

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

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 25, 2017

Contributor

React rails has the same issue reactjs/react-rails#607

Contributor

sevos commented Feb 25, 2017

React rails has the same issue reactjs/react-rails#607

@renchap

This comment has been minimized.

Show comment
Hide comment
@renchap

renchap Feb 25, 2017

Owner

Can you fork https://github.com/renchap/webpacker-react-example and share a branch showing with this behaviour?

From facebook/react#8928 (comment) I think Turbolinks reloads the pack file on visit, hence reloading React.

Owner

renchap commented Feb 25, 2017

Can you fork https://github.com/renchap/webpacker-react-example and share a branch showing with this behaviour?

From facebook/react#8928 (comment) I think Turbolinks reloads the pack file on visit, hence reloading React.

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 25, 2017

Contributor

It's just when we use turbolinks5 and unmount components on turbolinks:before-render, then as described in the ticket above, the warning happens. A quick solution would be to use turbolinks:before-cache to unmount the component. I know, the component won't get cached in turbolinks that way, but if this gets rid of these warnings, I would be fine.

Contributor

sevos commented Feb 25, 2017

It's just when we use turbolinks5 and unmount components on turbolinks:before-render, then as described in the ticket above, the warning happens. A quick solution would be to use turbolinks:before-cache to unmount the component. I know, the component won't get cached in turbolinks that way, but if this gets rid of these warnings, I would be fine.

@renchap

This comment has been minimized.

Show comment
Hide comment
@renchap

renchap Feb 25, 2017

Owner

As gaeron said in the comment I linked, it does not explain why React is complaining about "another version of React". This seems to be the real issue to fix, rather than a workaround.
I will try to reproduce it and have a look.

Owner

renchap commented Feb 25, 2017

As gaeron said in the comment I linked, it does not explain why React is complaining about "another version of React". This seems to be the real issue to fix, rather than a workaround.
I will try to reproduce it and have a look.

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 25, 2017

Contributor

To reproduce, you take your example app, you yarn link my branch of webpacker-react and launch everything. Then you run Turbolinks.visit(window.location.href) twice in the js console.

Contributor

sevos commented Feb 25, 2017

To reproduce, you take your example app, you yarn link my branch of webpacker-react and launch everything. Then you run Turbolinks.visit(window.location.href) twice in the js console.

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 25, 2017

Contributor

I think, I figured this out. I added few debugs and this is what I saw before the fix:

screen-shot-2017-02-25-18-31-30

On subsequent navigation before-render is called twice: once for cached page (with orphaned components) and once for the content loaded from the server. The warning happens when we try to unmount components loaded from the cache.

With the changes, we ensure that components are mounted after loading from the cache. This will ensure that they are still functioning, even when connection breaks.

After the change:

screen-shot-2017-02-25-18-37-06

Contributor

sevos commented Feb 25, 2017

I think, I figured this out. I added few debugs and this is what I saw before the fix:

screen-shot-2017-02-25-18-31-30

On subsequent navigation before-render is called twice: once for cached page (with orphaned components) and once for the content loaded from the server. The warning happens when we try to unmount components loaded from the cache.

With the changes, we ensure that components are mounted after loading from the cache. This will ensure that they are still functioning, even when connection breaks.

After the change:

screen-shot-2017-02-25-18-37-06

@renchap renchap referenced this pull request in reactjs/react-rails Feb 26, 2017

Closed

turbolinks:before-render for unmounting causes warnings #607

1 of 4 tasks complete
@renchap

This comment has been minimized.

Show comment
Hide comment
@renchap

renchap Feb 26, 2017

Owner

Nice find!
Do you think you can add a test for Turbolinks support? I think the simplest way would be to create a new controller with a Turbolinks layout, create a new pack that load Turbolinks, create 2 views in this controller, and have a test running visit on the first view, clicking on a link to the second, then to the first, and ensure that components are correctly loaded (and does not appear multiple times).

Owner

renchap commented Feb 26, 2017

Nice find!
Do you think you can add a test for Turbolinks support? I think the simplest way would be to create a new controller with a Turbolinks layout, create a new pack that load Turbolinks, create 2 views in this controller, and have a test running visit on the first view, clicking on a link to the second, then to the first, and ensure that components are correctly loaded (and does not appear multiple times).

sevos added some commits Feb 24, 2017

Support component mounting and unmounting in Turbolinks5, Turbolinks …
…2.4+, Pjax and native

React components will be mounted and unmounted on events specific for the technologies listed above.
This code is copied with modifications from https://github.com/reactjs/react-rails with modifications.
Mount components after render and once after load
Without this, components loaded from the cache were orphaned - not mounted in ReactDOM.
Also, we prevent mounting components twice on subsequent Turbolinks visits when "render" and "load" events are fired together.
@renchap

Could you please also add an entry to the (brand new) changelog?

I am ok to merge this once it has a test.

test/example_app/vendor/package.json
@@ -8,6 +8,7 @@
"devDependencies": {
"babel-core": "^6.22.1",
"babel-loader": "^6.2.10",
+ "babel-preset-es2015": "^6.22.0",

This comment has been minimized.

@renchap

renchap Feb 26, 2017

Owner

This is not needed, babel-preset-latest already includes it

@renchap

renchap Feb 26, 2017

Owner

This is not needed, babel-preset-latest already includes it

@@ -0,0 +1,76 @@
+const $ = (typeof window.jQuery !== 'undefined') && window.jQuery

This comment has been minimized.

@renchap

renchap Feb 26, 2017

Owner

Can you please add a comment saying this is adapted from rails-ujs?

@renchap

renchap Feb 26, 2017

Owner

Can you please add a comment saying this is adapted from rails-ujs?

sevos added some commits Feb 26, 2017

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 26, 2017

Contributor

Please note that I've had to make the init process explicit. IMHO it's better, importing a module should not have side effects.

Contributor

sevos commented Feb 26, 2017

Please note that I've had to make the init process explicit. IMHO it's better, importing a module should not have side effects.

sevos added some commits Feb 26, 2017

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 26, 2017

Contributor

Before releasing a new version, could you please take a look at the other PR, where we would make WebpackerReact.register to take an object? We could push these two changes in one release.

Additionally, we could let WebpackerReact.initialize to take such object as well, to reduce number of calls and make the API nicer.

Contributor

sevos commented Feb 26, 2017

Before releasing a new version, could you please take a look at the other PR, where we would make WebpackerReact.register to take an object? We could push these two changes in one release.

Additionally, we could let WebpackerReact.initialize to take such object as well, to reduce number of calls and make the API nicer.

@sevos

This comment has been minimized.

Show comment
Hide comment
@sevos

sevos Feb 26, 2017

Contributor

I am done in this branch. Feel free to merge and lets discuss further API changes ;)

Contributor

sevos commented Feb 26, 2017

I am done in this branch. Feel free to merge and lets discuss further API changes ;)

@renchap renchap merged commit 006e360 into renchap:master Feb 26, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@renchap

This comment has been minimized.

Show comment
Hide comment
@renchap

renchap Feb 26, 2017

Owner

Merged 👍

@sevos do you think it would be ok to call initialize on the first call to register? I would really like it to be called automatically to avoid any problems when users load multiple packs on the same page, so we need to ensure it is only called once in this case ; I am not sure of the behaviour of this.eventsRegistered when using multiple packs). Maybe we should replace this check by looking at the event listeners on document instead?

Feel free to send another PR with this change.

Owner

renchap commented Feb 26, 2017

Merged 👍

@sevos do you think it would be ok to call initialize on the first call to register? I would really like it to be called automatically to avoid any problems when users load multiple packs on the same page, so we need to ensure it is only called once in this case ; I am not sure of the behaviour of this.eventsRegistered when using multiple packs). Maybe we should replace this check by looking at the event listeners on document instead?

Feel free to send another PR with this change.

@szyablitsky szyablitsky referenced this pull request in shakacode/react_on_rails Mar 19, 2017

Merged

Fix turbolinks mount event #764

@renchap renchap referenced this pull request in rails/webpacker May 26, 2017

Closed

React + Turbolinks = Warning: unmountComponentAtNode() #336

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