(fix) ProgressBar state error

[mihir0x69]

"Expected" to fix #1505

Navigating between routes would throw Warning: setState(...): Can only update a mounted or mounting component. It could not be reproduced reliably. After applying several solutions, the current solution "seems" to have solved the problem because after navigating for a gazillion times, I couldn't reproduce the error.

Please test and let me know if it works. Also, if possible, record your activity so that it'll help me track down the real cause.

[gihrig]

LGTM! 💯

Clone
Checkout bfix/progress-state-error
yarn
npm run build
npm start

Progress bar works as expected on first visit to Features, also on revisit after language change.

No errors! ✨

Node 7.4.0
npm 4.1.1
yarn 0.19.1
Mac OS 10.11.6
Chrome 55.0.2883.95 (64-bit))

[gihrig]

LGTM! (Tested as shown in comment)

[smashercosmo]

@KarandikarMihir Could you please explain your solution? Cause I really don't get it. "bind" just returns bound function. How does it help in this case?

[mihir0x69]

@smashercosmo Yeah. in my case, the only way to get that error was to change the language from the dropdown in <Footer /> and then navigate away to a different route. Changing the language would not only re-render the <App />, it would re-mount it. (Sometimes I saw that <App /> would re-mount sporadically while navigating without changing the language, which made it even harder for me to understand the problem). But I knew that this is where I should be looking.

After googling for a while, I came to know that withProgressBar was trying to use a listener which was registered with an unmounted component to update the state of newly mounted component.

According to MDN-

Function.prototype.bind()

The bind() method creates a new function that, when called, has its this keyword set to the provided value, with a given sequence of arguments preceding any provided when the new function is called.

Return Value

A copy of the given function with the specified this value and initial arguments.

So basically, we'll get a new copy of router.listenBefore every time the component gets re-mounted. So I went for this solution.

BUT
I can't be 100% sure because of the unpredictable behavior of LanguageProvider or whatever that's causing the app to re-mount. What I need is as many people as we can have to test this. If you could, please test and let me know if it works at all. It is working for me.

[smashercosmo]

@KarandikarMihir O, I see you changed code, so now there is an actual assignment of the bound function) My question was basically about that)

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling c179552 on KarandikarMihir:bfix/progress-state-error into d898c43 on react-boilerplate:dev.

[gihrig]

Still LGTM 💯

Progress bar works as expected on first visit to Features, also on revisit to Features and Home after language change.

No errors! ✨

Node 7.5.0
npm 4.1.2
yarn 0.20.0
Mac OS 10.11.6
Chrome 56.0.2924.87 (64-bit)

[smashercosmo]

By the way, can't we just set this.mounted flag to true in componentDidMount and unset it in componentWillUnmount and then just use it in updateProgress function:

if (this.mounted) {
  this.setState({ progress })
}

I think this will be the simplest solution
@KarandikarMihir what do you think?

[samit4me]

I've been testing this and it was looking really good, but then after several attempts, I did manage to get the following error (hint: you got to be quick):

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the Router component.

Testing environment

Windows 10
Chrome 56
node 6.9.1
npm 3.10.8
yarn 0.19.1

Steps taken

1. git clone https://github.com/KarandikarMihir/react-boilerplate.git
2. git co bfix/progress-state-error
3. yarn
4. npm start
5. Open http://localhost:3000/ in Chrome
6. Open devtools
7. Select Network tab in devtools
8. Select GPRS in the throttling dropdown.
9. On the webpage, click the Features button.
10. Then quickly select "de" from the language dropdown in the footer
11. 💥 the error

Problem

After initial investigations, it looks like the issue has nothing to do with the withProgressBar() as I've removed it from the boilerplate and could still replicated the issue, so this problem has probably been around for some time.

I believe this issue exists because the <Router /> is a child of <LanguageProvider />. Therefore, when you select a different language it causes the <LanguageProvider /> to re-render, which causes <Router /> to unmount.

Unfortunately, react-router calls setState() inside of <Router />.componentWillMount(), see L102 of Router.js. Usually this doesn't cause any issues but if you change the language in the middle of a route transition, the setState() mentioned above is executed after <Router /> has been unmounted, which gives the error in question.

Possible solution

Not sure if it is possible, but if we could move <LanguageProvider /> to be a child of <Router /> we may have a better chance of resolving this problem. The current challenge is that the messages are passed into <LanguageProvider /> as props are hot reloadable.

My initial thoughts are to move these hot messages into the Redux store, but would love to hear other peoples thoughts on this???

Shall we move this to a new issue, so we can discuss without the confusion of the ProgressBar???

[samit4me]

@KarandikarMihir @gihrig Did we want to merge these changes? LGTM, but maybe we should leave the original issue open and look into resolving it is a separate PR???

[smashercosmo]

@samit4me Why would you want to merge these changes if they are not fixing the issue?

[samit4me]

The change was already approved, so I thought it might be best to ask those involved!

[gihrig]

Why would you want to merge these changes if they are not fixing the issue?

Because It improves the issue to the point where it was not observed with the original testing.

I think it makes sense to merge as is (because it improves the issue) with the understanding there remains an issue that will appear on combination of a specific action and a slow connection. An "edge case"?

So, I would merge, but keep #1505 open for a better fix.

[mihir0x69]

@smashercosmo I tried your way, but no joy :(

@samit4me Thanks for digging in! That was spot on! I removed appWithRouter, followed your steps and saw the error. I knew something was fishy with <LanguageProvider />.

But that being said, this PR provides some value because it improves the code a little bit- if anything, it's harmless. I've observed that the frequency of setState error has decreased after this fix but I don't have a definitive proof. So I say let's merge this in. Basically what @gihrig said.

Sorry for the delay. I've been busy with work. Let's continue discussion in #1505 and hopefully we'll solve the problem by deductive reasoning.

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling 4d28aab on KarandikarMihir:bfix/progress-state-error into 9cfdf6d on react-boilerplate:dev.

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling 4d28aab on KarandikarMihir:bfix/progress-state-error into 9cfdf6d on react-boilerplate:dev.

[samit4me]

Awesome, let's get this merged!

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling 9f62a0b on KarandikarMihir:bfix/progress-state-error into f29a78c on react-boilerplate:dev.

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling e172a0d on KarandikarMihir:bfix/progress-state-error into 3c1bd28 on react-boilerplate:dev.

[coveralls]

Coverage increased (+0.003%) to 99.242% when pulling e172a0d on KarandikarMihir:bfix/progress-state-error into 3c1bd28 on react-boilerplate:dev.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.