Skip to content
This repository was archived by the owner on May 15, 2019. It is now read-only.

Commit 1a68aa9

Browse files
committed
[sat fixit] Don't call setState for unmounted components
Summary: Before this change, in very rare cases, `component.setState()` was being called on components that weren't actually mounted. In particular, this happened in SAT when: 1. A component (call it `A`) was subscribed to a store using `StateFromStoreMixin` and triggered a flux change in that store to navigate to a new URL. 2. Backbone started updating the `StateFromStoreMixin`s that were subscribed to that store. 3. The parent component of `A` received the update, re-rendered, and `A` was no longer in the render tree, and was unmounted. 4. Backbone continued updating the `StateFromStoreMixin` instances, eventually reaching `A`'s instance. 5. `A`'s `StateFromStoreMixin` attempted to call `.setState()` to update to the new values, but `A` is no longer mounted. The solution to this is for `StateFromStore` to check whether the component is mounted before calling `.setState()`. We could do this using `component.isMounted()` but [`.isMounted` is an antipattern](https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html) and will be deprecated. Instead, we manually keep track of an `isMounted` value (as suggested by the article) and check that. Test Plan: - Visit http://localhost:8081/mission/sat/practice/math - Open a practice task - See no warning about `setState()` being called on an unmounted component. Reviewers: amy, mattdunnrankin Reviewed By: amy Subscribers: lauren, #gtp Differential Revision: https://phabricator.khanacademy.org/D34577
1 parent 539b056 commit 1a68aa9

File tree

1 file changed

+8
-3
lines changed

1 file changed

+8
-3
lines changed

js/state-from-store-mixin.jsx

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,14 @@
4343

4444
const StateFromStore = function(stateDescriptors) {
4545
const storageKey = "StateFromStoreMixin" + (new Date).getTime();
46+
let isMounted = false;
4647

4748
const setState = function(component, stateKey, stateData) {
48-
const newState = {};
49-
newState[stateKey] = stateData;
50-
component.setState(newState);
49+
if (isMounted) {
50+
const newState = {};
51+
newState[stateKey] = stateData;
52+
component.setState(newState);
53+
}
5154
};
5255

5356
/**
@@ -155,11 +158,13 @@ const StateFromStore = function(stateDescriptors) {
155158
},
156159

157160
componentDidMount: function() {
161+
isMounted = true;
158162
addChangeListeners(this);
159163
},
160164

161165
componentWillUnmount: function() {
162166
removeChangeListeners(this);
167+
isMounted = false;
163168
},
164169

165170
componentWillReceiveProps: function(nextProps) {

0 commit comments

Comments
 (0)