1074547 - remove direct manipulation of this.state in react components

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

There's a pattern that's used in the contacts code (for example at <https://github.com/mozilla/gecko-dev/blob/master/browser/components/loop/content/js/contacts.jsx#L137>) which is vulnerable to state corruption.  See the red Notes box on <http://facebook.github.io/react/docs/component-api.html> for details.

I've filed a feature request on React asking for a warning in the development version of React so this will be easier to inadvertently avoid in the future.

As it stands, I think it makes sense to fix our code and request uplift for it so that we don't end up with hard-to-diagnose/debug issues in the stuff that rides the trains to release channel.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Oh yeah, the React.js issue is <https://github.com/facebook/react/issues/2272>

Comment 2

[:Paolo Amadini]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #0)
> There's a pattern that's used in the contacts code (for example at
> <https://github.com/mozilla/gecko-dev/blob/master/browser/components/loop/
> content/js/contacts.jsx#L137>) which is vulnerable to state corruption.  See
> the red Notes box on
> <http://facebook.github.io/react/docs/component-api.html> for details.

I'm not sure what you're seeing here. The note is about the fact you should use setState instead of setting properties on "state" directly, which is what we're doing in the code you pointed to.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Whoops, my link pointed to the wrong line.  The issue is contacts[guid] = contact, which effectively reaches into the this.state and mutates this.state.contacts:

handleContactAddOrUpdate: function(contact) {
  let contacts = this.state.contacts;
  let guid = String(contact._guid);
  contacts[guid] = contact;
  this.setState({});
},

If there's already a previously queued change to this.state.contacts which hasn't yet been applied, I believe it could be applied after this function executes, overwriting the changes made here.

Comment 4

[:Paolo Amadini]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #3)
> If there's already a previously queued change to this.state.contacts which
> hasn't yet been applied, I believe it could be applied after this function
> executes, overwriting the changes made here.

You're right, this may happen when all the contacts are removed.

For all the other operations, "contacts" acts like an external data source - in fact I'm not sure this should be stored in the "state" at all.

Comment 5

[:Paolo Amadini]

By the way, <https://github.com/facebook/react/issues/2272> would not help here, as we're still only reading from "this.state".

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to :Paolo Amadini from comment #4)
> (In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for
> response from comment #3)
> > If there's already a previously queued change to this.state.contacts which
> > hasn't yet been applied, I believe it could be applied after this function
> > executes, overwriting the changes made here.
> 
> You're right, this may happen when all the contacts are removed.

It's also a fragility thing.  I have yet to see this pattern in other react code (though, admittedly, I've not spent lots of time reading such), and if test code or if someone else in the future uses the normal pattern (which seems highly likely), I think we'd be notably more likely to hit this.

> For all the other operations, "contacts" acts like an external data source -
> in fact I'm not sure this should be stored in the "state" at all.

It's true that it comes from an external data source, but it's not obvious to me what we'd do with it today, since it realistically is part of the state of the widget.  That said, the FLUX pattern should move the contacts themselves to the store component, and the controller should then be able pass this dictionary in as a prop, which seems like a better step.

Since fixing the pattern is easy, I feel like the thing to do is just go forward with that...

(In reply to :Paolo Amadini from comment #5)
> By the way, <https://github.com/facebook/react/issues/2272> would not help
> here, as we're still only reading from "this.state".

Heh, you're quite right.  Whoops!  I still think it'd be a good feature for React, though.  :-)

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to :Paolo Amadini from comment #5)
> By the way, <https://github.com/facebook/react/issues/2272> would not help
> here, as we're still only reading from "this.state".

Hmm:

let contacts = this.state.contacts;
let guid = String(contact._guid);
contacts[guid] = contact;
this.setState({});

`contacts[guid] = contact` mutates the contacts object attached to state here.

Also, there's this convenient forceUpdate() method, instead of setState({}).

I'm also concerned shouldComponentUpdate is not implemented for contact entries, so we might easily get into a situation where the whole set of contact entry components is rerendered - which is possibly circumvented by the fact that you're avoiding calling setState with the right, updated contacts object. But this not the "react" way of doing things if you ask me.

Comment 8

[Mike de Boer [:mikedeboer]]

I noticed a severe slowdown when importing > 100 contacts and rendering those during a fresh panel load.

I'll make the necessary changes that'll keep performance on target and implement `componentShouldUpdate` for ContactDetail.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: improve rendering flow of contacts list

Comment 10

[Marco Mucci [:MarcoM]]

Added to IT 35.3

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8498093 [details] [diff] [review]
Patch v1: improve rendering flow of contacts list

Review of attachment 8498093 [details] [diff] [review]:
-----------------------------------------------------------------

r=me conditional to minor comments made here and on IRC being addressed.

::: browser/components/loop/content/js/contacts.jsx
@@ +263,5 @@
>          });
>        });
>      },
>  
> +    handleContactAddOrUpdate: function(contact, update = true) {

`update` is probably misleading here, how about `render`?

Comment 12

[Mike de Boer [:mikedeboer]]

Thanks! Pushed to fx-team with comments addressed: https://hg.mozilla.org/integration/fx-team/rev/cd7aa281060d

Comment 13

[:Paolo Amadini]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #3)
> If there's already a previously queued change to this.state.contacts which
> hasn't yet been applied, I believe it could be applied after this function
> executes, overwriting the changes made here.

The patch here didn't solve this issue... we still mutate the state on remove all.

With regard to using forceUpdate versus setState({}), there was a brief exchange between me and Mike when the patch was first implemented, and we used setState as it wasn't clear from the documentation whether forceUpdate was executed synchronously. In the case of importing many contacts, which goes through the normal API, it could have been useful to batch the DOM additions. But the approach in this patch may also work.

Implementing componentShouldUpdate should be unnecessary, as one of the properties of ReactJS is that render is cheap so this kind of explicit state checking can be avoided. Also, has it any effect on the "menu open" state handling?

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #13)
> The patch here didn't solve this issue... we still mutate the state on
> remove all.

Yes, but the removeAll listener was added for testing purposes.

> With regard to using forceUpdate versus setState({}), there was a brief
> exchange between me and Mike when the patch was first implemented, and we
> used setState as it wasn't clear from the documentation whether forceUpdate
> was executed synchronously. In the case of importing many contacts, which
> goes through the normal API, it could have been useful to batch the DOM
> additions. But the approach in this patch may also work.

Well, this patch takes care of the initial loading on panel open for now. I remember our discussion vividly, but Niko` assured me this is what we're supposed to use.

> Implementing componentShouldUpdate should be unnecessary, as one of the
> properties of ReactJS is that render is cheap so this kind of explicit state
> checking can be avoided. Also, has it any effect on the "menu open" state
> handling?

Well, that might be true, but it doesn't hurt as well. My gut feeling says it'll speed up a re-import - which mostly adds contacts that were already in the list - because it won't call into render() for contacts that are already there.
What kind of effect did you expect or were you hoping for wrt the 'menu open' state handling?

Comment 15

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> What kind of effect did you expect or were you hoping for wrt the 'menu
> open' state handling?

I'd have expected that the menu did not open, as you're only checking props and not state.

But this doesn't happen because you called the method componentShouldUpdate instead of shouldComponentUpdate - the method isn't called at all :-)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd7aa281060d

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8498093 [details] [diff] [review]
Patch v1: improve rendering flow of contacts list

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16b361184a45

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8498093 [details] [diff] [review]
Patch v1: improve rendering flow of contacts list

Already landed, tacking for posterity.