1435991 - Document choosing between pure function, pure component and component

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Bug 1434848 highlighted that pure function can be a performance bottleneck when the component has no parent to prevent it from render too many times.
But work on the console also proved pure function can provide a perf improvement by removing component mount/update work has to do with components.

At the same time, Pure components can be an alternative, but it isn't a magic bullet either and can be a slow if used with objects with a lot of deep/nested objects.

It would be helpful to have a short paragraph somewhere in devtools perf doc to help choosing the best option depending on each scenario.

A new page under or next this one:
  http://docs.firefox-dev.tools/contributing/performance.html
would be a great place for such writings.

A "performance best practices" page for example?

Comment 1

[Julien Wajsberg [:julienw]]

> At the same time, Pure components can be an alternative, but it isn't a magic bullet either and can be a slow if used with objects with a lot of deep/nested objects.

Just a note that a PureComponent doesn't deeply compare objects, it compares only shallowly [1]. This is strictly identical to the old shallow-compare util [2].

[1] https://reactjs.org/docs/react-api.html#reactpurecomponent
[2] https://facebook.github.io/react/docs/shallow-compare.html

Comment 2

[Julien Wajsberg [:julienw]]

I'll try to provide something.

Comment 3

[Yulia Startsev [:yulia] OOO until July 2026]

I noticed we are using terms in different ways. Here is a quick write up so that we can have a shared language:

There are three potential ways to do components. There is no significant change with these three types in react 16, stateless components remain as performant as stateful components [3], and Pure components are more efficient. It is important to do benchmarks with production settings as well. Pure components should be preferred as they are more efficient [2] but they come with their own drawbacks

A bit of clarification about the terms and the underlying logic:

stateful components --
extend the "Component" from the react library. This was one of the earliest ways to make react components. It looks like this:

```
import { Component } from "react";

class FooComponent extends Component {
  constructor(props) {
    super(props);
    ...
  }
  
  // other life cycle methods
  
  // other methods

  render() {
    return < ... >;
  }
}
```

Pros:
- does a deep check of changes to the props.

cons:
- slow

stateless, or "functional" components --
consist of a single function with no accessible lifecycle methods, they do not extend any class from the react library, but are instead wrapped in a class when run. It looks like this:

function FooComponent(props) {
  return <div>props.someString</div>;
}

Pros:
- does a deep check of changes to the props.
- is more readable and useful for small components that render simple data

cons:
- slow
- less control over lifecycle

There is a hack to make this faster but it isn't endorsed by anyone from the react team and I suspect it has unknown drawbacks, which is to not use the jsx way of instantiating the component, but just treating it as a function:

```
<div>{MyComponent()}</div>
// instead of 
<MyComponent />
```

This needs more investigation... not sure what to think about this for now

Pure components - 
are also stateful, but extend the "PureComponent" from the react library. The key difference is that they do not rerender if there are no changes to the top level props or state, unlike both stateless and stateful components. There are also changes to shouldComponentUpdate (see [1] for more info) It looks like this:

```
import { PureComponent } from "react";

class MyComponent extends PureComponent {
  constructor(props) {
    super(props);
    ...
  }
  
  // other life cycle methods
  
  // other methods

  render() {
    return < ... >;
  }
}
```

Part of what makes this fast, is like Julien mentioned -- it doesn't do a deep compare like the regular component. It's only comparing shallow values. This isn't an issue in most cases, especially if you are working in an immutable style and always returning a new object with a different identity each time there is an update. But it has the potential to have weird side effects such as the view not updating when the data changes if you are not careful. [1]


Pros:
- faster than the other two methods

Cons:
- Does a shallow check
- encourages potentially memory intensive practices, such as copying an object's references to a new object when making a change to one of its properties

I suspect there was a bit of mixing of the concepts between pure and functional components. It also doesnt help that there are multiple names for everything. There is one last thing i want to mention regarding speed ups that might have happened if indeed we were using funcitonal/stateless components and saw an increase:

doing logic in render functions

If you are doing logic in render functions, what happens is that the whole process slows down. here is an example:

```
class MyComponent extends Component {
  // some stuff

  render() {
    const someComputedValue = (function () {
      return Math.rand();
    }
    return <div>{someComputedValue}</div>
  }
}
```

This will slow down the render cycle, and we want to avoid it in all cases. However -- stateless/functional components avoid this, because they treat the return value as the render. the only way to slow it down in that case is if you do something like this [4]:

```
function MyComponent(props) {
  return <button onClick={() => props.onClick()}> my button </button>
}
```

This will also cause a slow down.

A couple of takeaways:
- avoid inline functions in renders in all cases
- use good shouldComponentUpdate rules if necessary
- prefer shallow data structures, only pass what you need (this isn't always possible)
- prefer a pure component


references:
[1] https://60devs.com/pure-component-in-react.html
[2] https://moduscreate.com/blog/react_component_rendering_performance/
[3] https://twitter.com/dan_abramov/status/755343749983657986?ref_src=twsrc%5Etfw&ref_url=https%3A%2F%2Fmoduscreate.com%2Fblog%2Freact_component_rendering_performance%2F
[4] https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

Comment 4

[Julien Wajsberg [:julienw]]

Thanks for the write-up Yulia !

I have some small comments though.

1. You write that for Components/Stateless components, it does a deep check of changes to the props. I believe this is a misunderstanding: it does no check at all if you don't implement `shouldComponentUpdate`. Without `shouldComponentUpdate`, what it does is that it renders, and then does a virtual-dom diff to decide what to update in the (real) DOM. From the outside it looks as if it does a deep check, but it's not. It's worse ;)

2. About `PureComponent`, you write that "there are changes to `shouldComponentUpdate`"; that's really that it just has an implementation :) If you want to implement this method yourself, just use a `Component`.

3. Your explanations around how to slow down `render` are interesting, especially the difference between a Component and a Stateless Component. Do you have a source for this? I'd be interested to know more!

4. About the inline function as prop that you mention at the end, this is not a big perf issue for DOM elements like in your example. This is an issue for PureComponents because the shallow comparison will always be false. For DOM elements we don't have this issue, but this still can be a memory issue because a new function is created at each `render`.

Comment 5

[Yulia Startsev [:yulia] OOO until July 2026]

Ah you made some really good points. Thanks for clearing it up!

I think i might have miss-read the old code that implemented stateless components... so maybe disgregard point 2 -- here is the code i was thinking of but reading it again i think i might have made a mistake: https://github.com/facebook/react/blob/e5c3fb14fa3427b331b66785d94aef404ce66156/src/renderers/shared/reconciler/ReactCompositeComponent.js#L43

Even there it would run all of the code in a stateless component as though it was a render function so... ignoooree

Comment 6

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Julien, are you still planning to write this doc? I think Yulia unassigned you by mistake while replying to your comment.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

A couple of points that should be added:

1. When using arr.map(blah) we really should add keys and these keys should be unique strings where possible. If we don't do this React uses integers as keys and any time something changes it reassigns keys to the complete set of nodes.
2. When calling setState() we should try to use the callback parameter where possible. This is particularly importand if you call setState() and then a couple of lines down access this.state. Ideally we should always use a callback if there is code after setState() in order to avoid introducing async issues in the future. Reacts path is going to become more and more async so this will become much more important in the future.

Comment 8

[Julien Wajsberg [:julienw]]

> Julien, are you still planning to write this doc? I think Yulia unassigned you by mistake while replying to your comment.

Yeah it's still on my plate. I didn't forget but was working on other work. I'll write a first draft before the end of this week.

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #7)
> A couple of points that should be added:

Your 2 points could be checked using eslint rules, therefore we should enable these rules if possible, rather than write this down to the doc.

> 
> 1. When using arr.map(blah) we really should add keys and these keys should
> be unique strings where possible. If we don't do this React uses integers as
> keys and any time something changes it reassigns keys to the complete set of
> nodes.

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-key.md

> 2. When calling setState() we should try to use the callback parameter where
> possible. This is particularly importand if you call setState() and then a
> couple of lines down access this.state. Ideally we should always use a
> callback if there is code after setState() in order to avoid introducing
> async issues in the future. Reacts path is going to become more and more
> async so this will become much more important in the future.

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-access-state-in-setstate.md
OK, that one is not the same as what you say, but I think this is more important.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Bug 1435991 - Document React component best usage

Review commit: https://reviewboard.mozilla.org/r/222716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222716/

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

I haven't proofread my writing yet so things may be somewhat rough, but I'd like some feedback about the general tips, and maybe the structure of the text too.

Comment 11

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review228672

::: devtools/docs/contributing/react-performance-tips.md:10
(Diff revision 1)
> +
> +## How React renders normal components
> +
> +As a start let's discuss about how React renders normal plain components, that
> +don't use `shouldComponentUpdate`. What we call plain components here are either:
> +* classes that extends [`Component`](https://reactjs.org/docs/react-component.html)

nit: "extends" should be "extend"

::: devtools/docs/contributing/react-performance-tips.md:33
(Diff revision 1)
> +updates to the DOM the less work the browser has to do to reflow and repaint the
> +application.
> +
> +### Main performance issue sources
> +
> +From this simple explanation we can gather that the main performance issues can

nit: `simple` could be dropped from this sentence

::: devtools/docs/contributing/react-performance-tips.md:43
(Diff revision 1)
> +   authors, which means the processing duration increases linearly with the number
> +   of elements in the tree we compare.
> +
> +#### Do not render too often
> +
> +As said above a rerender will happen after calling `setState` to change the

nit: `As said above` can be removed from this sentence.

I was also under the impression that a component will rerender if its props change. is this still current? http://busypeoples.github.io/post/react-component-lifecycle/

::: devtools/docs/contributing/react-performance-tips.md:57
(Diff revision 1)
> +#### Render methods
> +
> +When rendering a list, it's very common that we'll map this list to a list of
> +components. This can be costly and we might want to cut this list in several
> +chunks of items or to
> +[virtualize this list](https://reactjs.org/docs/optimizing-performance.html#virtualize-long-lists).

neat!

::: devtools/docs/contributing/react-performance-tips.md:61
(Diff revision 1)
> +chunks of items or to
> +[virtualize this list](https://reactjs.org/docs/optimizing-performance.html#virtualize-long-lists).
> +Although this is not always possible.
> +
> +Do not do heavy computations in your `render` methods. Rather do them before
> +setting the state, and set the state to the resulf ot these computations.

nit: "resulf" should be "result"

::: devtools/docs/contributing/react-performance-tips.md:64
(Diff revision 1)
> +
> +Do not do heavy computations in your `render` methods. Rather do them before
> +setting the state, and set the state to the resulf ot these computations.
> +Ideally `render` should be a direct mirror of the component's props and state.
> +
> +#### Help the reconciliation algorithm being efficient

nit: "being" should be "be"

::: devtools/docs/contributing/react-performance-tips.md:80
(Diff revision 1)
> +[react/jsx-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-key.md),
> +part of the recommended rules of [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react/),
> +and
> +[react/no-array-index-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md).
> +
> +## `shouldComponentUpdate` and `PureComponent`: avoiding renderings altogether

these two probably need their own separate sections, while they do the same thing, the reader will have to do a mental context switch if they are new to the subject. shouldComponentUpdate might be a nice bridge between the "normal component' and the "pure component"

::: devtools/docs/contributing/react-performance-tips.md:116
(Diff revision 1)
> +common cases are:
> +
> +* Using a bound function or an arrow function as a prop:
> +
> +  ```javascript
> +  render() {

aaahhh!

::: devtools/docs/contributing/react-performance-tips.md:140
(Diff revision 1)
> +### TL;DR tips
> +
> +* Prefer props and state immutability and use `PureComponent` components as a default
> +* As a convention, the reference should change if and only if the inner data
> +  changes.
> +  * Never use bound or arrow functions as props to a Component (it's fine to use

won't it do the same if you pass an anonymous function to a child component?

Comment 12

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review228672

Nice! a few nits re: english :)

Comment 13

[Julien Wajsberg [:julienw]]

Thanks Yulia, all your comments are good, I'll change this on monday.

One thing about this:
> I was also under the impression that a component will rerender if its props change. is this still current? http://busypeoples.github.io/post/react-component-lifecycle/

This is still current, and a very important part of a React/Redux app, but because in this first part I'm focussing on a pure react app (no redux especially), I thought that a component's props can't change "by itself", but only as a result of a parent component's state change. That's why I didn't want to mention it very explicitely, although I write implicitely:

> When that happens, React will call that component's `render` method, and then
> recursively call every child's `render` method, possibly with changed props
> compared to the previous render.


Once we start discussing redux in the next part, the same happens as the connected component's state changes when the redux store changes ([1]) but this is completely hidden, so I think this is the right place to mention this more.

[1] https://github.com/reactjs/react-redux/blob/fd436b1e024c4194d3f44dfcef8e30fd24a99a1a/src/components/connectAdvanced.js#L218

What do you think? Do you think I should be more explicit about it in this section?

Comment 14

[Yulia Startsev [:yulia] OOO until July 2026]

Thanks for explaining your thinking!

I think the place where I tripped up was here: 

"
There are 2 ways to trigger a render:
1. We call `ReactDOM.render`.
2. One component's state changes ...
" 


this list is followed with "When that happens" -- due to how the text is structured (the last text on the same level ), I read this as "when a render is triggered" rather than referring specifically to point 2, and when I scanned the text again, I skipped that paragraph, since it functions as an elaboration rather than a significant point.

To keep the structure of the text, maybe we can make this clearer by changing the "when that happens" section to "When a state change occurs ..."

It might be a good idea to add a point 3 (maybe as 2a ;P) just in case, while its not different from point 2, i think its an easy place for people to mess up

Great work by the way!

Comment 15

[Julien Wajsberg [:julienw]]

> this list is followed with "When that happens" -- due to how the text is structured (the last text on the same level ), I read this as "when a render is triggered" rather than referring specifically to point 2, and when I scanned the text again, I skipped that paragraph, since it functions as an elaboration rather than a significant point.

Because this works the same with ReactDOM.render too [1]. You can call ReactDOM.render on the same DOM element and triggers a rerender on the tree. But I admit this isn't frequently used, especially for the devtools. I'll see how I can rework this to make it more obvious.

[1] https://reactjs.org/docs/react-dom.html#render "If the React element was previously rendered into container, this will perform an update on it and only mutate the DOM as necessary to reflect the latest React element."

Comment 16

[Julien Wajsberg [:julienw]]

> these two probably need their own separate sections, while they do the same thing, the reader will have to do a mental context switch if they are new to the subject. shouldComponentUpdate might be a nice bridge between the "normal component' and the "pure component"

I believe `PureComponent` should be used everywhere :-), that's why I didn't want to first explain shouldComponentUpdate. I feel like PureComponent should be the normal case, and shouldComponentUpdate the exception.

But I understand what you say and I'll see what I can do to make it clearer.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/1-2/

Comment 18

[Julien Wajsberg [:julienw]]

I updated the document, taking Yulia's comment into account and adding some code.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/2-3/

Comment 20

[Greg Tatum [:gregtatum]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review230102

Can you link out early to the official React docs on performance? They seem to be quite nice. Also, I don't get a good sense from reading this how to limit component re-renders, and how a tree structure can help stop changes propagating across the entire tree.

::: devtools/docs/contributing/react-performance-tips.md:75
(Diff revision 3)
> +then a virtual DOM tree. It will then render actual DOM elements to the
> +specified container.
> +
> +### Subsequent rerenders
> +
> +There are 3 ways to trigger a rerender:

I think it's worth mentioning connected components here as another way to trigger a re-render.

::: devtools/docs/contributing/react-performance-tips.md:99
(Diff revision 3)
> +[_reconciliation_](https://reactjs.org/docs/reconciliation.html) to find the
> +minimal set of updates to apply to the DOM. This is good because the less
> +updates to the DOM the less work the browser has to do to reflow and repaint the
> +application.
> +
> +### Main performance issue sources

"Main sources of performance issues"

This sounds a bit more natural.

::: devtools/docs/contributing/react-performance-tips.md:103
(Diff revision 3)
> +
> +### Main performance issue sources
> +
> +From this explanation we can gather that the main performance issues can
> +come from:
> +1. rendering too often,

Not sure what the difference really is between 1 and 2.

::: devtools/docs/contributing/react-performance-tips.md:104
(Diff revision 3)
> +### Main performance issue sources
> +
> +From this explanation we can gather that the main performance issues can
> +come from:
> +1. rendering too often,
> +2. calling render methods,

"unnecessarily calling render methods"

::: devtools/docs/contributing/react-performance-tips.md:134
(Diff revision 3)
> +setting the state, and set the state to the result to these computations.
> +Ideally `render` should be a direct mirror of the component's props and state.
> +
> +#### Help the reconciliation algorithm be efficient
> +
> +As said earlier, the smaller the tree is, the faster the algorithm is. So it's

I disagree with this statement or am misunderstanding it. The tree or a subtree can be arbitrarily large, but what's important is the finite list of nodes that need to be re-rendered. A parent node can recreate the children, but those children may not have different props, so they will in turn not re-render.

e.g. assuming all pure components:

Here A could re-render, but if B received the same props and did not re-render, then B's render will never run. So we only have 1 render call, and 2 checks for props being shallow equal.

```
        A
      /   \
     B     B
    /|\   /|\
   C C C C C C
```

::: devtools/docs/contributing/react-performance-tips.md:251
(Diff revision 3)
> +
> +For objects, it means you don't change/add/delete inner properties directly:
> +
> +```javascript
> +this.setState(state => {
> +  // NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO

This feels a bit much.

::: devtools/docs/contributing/react-performance-tips.md:281
(Diff revision 3)
> +
> +Keep reading to learn how to proceed.
> +
> +#### Keep your state objects simple
> +
> +Updating your immutable state objects can be difficult if the objects used are

For me Redux solves the complexity issue by providing combineReducer.

::: devtools/docs/contributing/react-performance-tips.md:287
(Diff revision 3)
> +complex. That's why it's a good idea to keep the objects simple, especially keep
> +them not nested, so that you don't need to use a library like
> +[immutability-helper](https://github.com/kolodny/immutability-helper),
> +[updeep](https://github.com/substantial/updeep), or even
> +[Immutable](https://github.com/facebook/immutable-js). Be especially careful
> +with Immutable as it's easy to shoot yourself in the foot with it.

I'd state, "it's easy to create performance problems using it." or elaborate what you mean by this without a metaphor.

Comment 21

[Greg Tatum [:gregtatum]]

We should probably mention how to diagnose performance issues with profiling and instrumenting and different tricks.

* React's built-in performance.mark and measure
* Adding console.logs to render methods, and then clicking on random parts of your app to see what re-renders.
* Turning on paint flashing and doing the same.

Comment 22

[Julien Wajsberg [:julienw]]

Thanks for all the mindful comments!

Some answers:

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #20)
> Comment on attachment 8953479 [details]
> Bug 1435991 - Document React component best usage
> 
> https://reviewboard.mozilla.org/r/222716/#review230102
> 
> Can you link out early to the official React docs on performance? They seem
> to be quite nice.

I found them nice about all the "shouldComponentUpdate" part but not very good about the rest, that's why I linked to them only in that part.

> Also, I don't get a good sense from reading this how to
> limit component re-renders, and how a tree structure can help stop changes
> propagating across the entire tree.

I'm not understanding this comment very well but I guess this is about how shouldComponentUpdate and PureComponents can "cut off" a complete branch of a tree. I'll elaborate a bit more about this.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:75
> (Diff revision 3)
> > +then a virtual DOM tree. It will then render actual DOM elements to the
> > +specified container.
> > +
> > +### Subsequent rerenders
> > +
> > +There are 3 ways to trigger a rerender:
> 
> I think it's worth mentioning connected components here as another way to
> trigger a re-render.

I wanted to avoid mentioning Redux too much in this part, especially at the start. I left a part dedicated to Redux usage at the end of the documentation, with a "TODO"...

As a matter of fact connected components actually just change the connected component's state internally, to trigger a render.  Moreover from the developer's point of view a connected component change the wrapped component's props, which is point 3 in this list. I guess I can still add a note here, just like I added a note about props...

> ::: devtools/docs/contributing/react-performance-tips.md:103
> (Diff revision 3)
> > +
> > +### Main performance issue sources
> > +
> > +From this explanation we can gather that the main performance issues can
> > +come from:
> > +1. rendering too often,
> 
> Not sure what the difference really is between 1 and 2.

Each line is detailed in the 3 next parts.
Basically, 1 is about triggering a rerender, while 2 is about expensive render methods.


> 
> ::: devtools/docs/contributing/react-performance-tips.md:104
> (Diff revision 3)
> > +### Main performance issue sources
> > +
> > +From this explanation we can gather that the main performance issues can
> > +come from:
> > +1. rendering too often,
> > +2. calling render methods,
> 
> "unnecessarily calling render methods"

No, this is especially about making render methods very lean and straightforward. This part was part of my first draft and this is likely too terse, I'll make this clearer.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:134
> (Diff revision 3)
> > +setting the state, and set the state to the result to these computations.
> > +Ideally `render` should be a direct mirror of the component's props and state.
> > +
> > +#### Help the reconciliation algorithm be efficient
> > +
> > +As said earlier, the smaller the tree is, the faster the algorithm is. So it's
> 
> I disagree with this statement or am misunderstanding it. The tree or a
> subtree can be arbitrarily large, but what's important is the finite list of
> nodes that need to be re-rendered. A parent node can recreate the children,
> but those children may not have different props, so they will in turn not
> re-render.
> 
> e.g. assuming all pure components:
> 
> Here A could re-render, but if B received the same props and did not
> re-render, then B's render will never run. So we only have 1 render call,
> and 2 checks for props being shallow equal.
> 
> ```
>         A
>       /   \
>      B     B
>     /|\   /|\
>    C C C C C C
> ```

In this part we haven't discussed about shouldComponentUpdate or PureComponents yet. You're right that this could alleviate the issue, I'll mention it, and I'll also make it clearer when I start discussing them.


> 
> ::: devtools/docs/contributing/react-performance-tips.md:251
> (Diff revision 3)
> > +
> > +For objects, it means you don't change/add/delete inner properties directly:
> > +
> > +```javascript
> > +this.setState(state => {
> > +  // NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO
> 
> This feels a bit much.

OK I'll write a nicer sentence :-)

> 
> ::: devtools/docs/contributing/react-performance-tips.md:281
> (Diff revision 3)
> > +
> > +Keep reading to learn how to proceed.
> > +
> > +#### Keep your state objects simple
> > +
> > +Updating your immutable state objects can be difficult if the objects used are
> 
> For me Redux solves the complexity issue by providing combineReducer.

I mention it later in "a few words about Redux". I didn't want to mention Redux here and only focus on states kept in components.
Moreover even with Redux it's necessary to keep the substate handled by one single reducer simple. Redux only solves the problem of how to combine various pieces of state together.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:287
> (Diff revision 3)
> > +complex. That's why it's a good idea to keep the objects simple, especially keep
> > +them not nested, so that you don't need to use a library like
> > +[immutability-helper](https://github.com/kolodny/immutability-helper),
> > +[updeep](https://github.com/substantial/updeep), or even
> > +[Immutable](https://github.com/facebook/immutable-js). Be especially careful
> > +with Immutable as it's easy to shoot yourself in the foot with it.
> 
> I'd state, "it's easy to create performance problems using it." or elaborate
> what you mean by this without a metaphor.

OK I'll try to elaborate a bit. But again I wanted to keep this document lean, by not entering into much details for things that are less important.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/3-4/

Comment 24

[Julien Wajsberg [:julienw]]

I pushed a new version taking Greg's comments into account.

Comment 25

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review230596

::: devtools/docs/contributing/react-performance-tips.md:14
(Diff revision 4)
> +* As a convention, the reference should change **if and only if** the inner data
> +  changes.
> +  * Never use bound or anonymous functions as props to a Component (it's fine to use
> +    them as props to a DOM element).
> +  * Be careful to not update a reference if the inner data doesn't change.
> +* Always measure before optimizing to have a real impact on performance.

maybe adding a link to Alex's performance docs is a good idea here http://docs.firefox-dev.tools/contributing/performance.html

::: devtools/docs/contributing/react-performance-tips.md:38
(Diff revision 4)
> +  ```jsx
> +  function Application(props) {
> +    return <div>{props.content}</div>;
> +  }
> +  ```
> +  These functions are actually equivalent to classes extending `Component`. In

nit: actually can be dropped from this sentence

::: devtools/docs/contributing/react-performance-tips.md:39
(Diff revision 4)
> +  function Application(props) {
> +    return <div>{props.content}</div>;
> +  }
> +  ```
> +  These functions are actually equivalent to classes extending `Component`. In
> +  the rest of the article we'll especially focus on the latter. Unless said 

nit: replace "Unless said otherwise" with "Unless otherwise stated" (feels more natural in this sentence)

::: devtools/docs/contributing/react-performance-tips.md:40
(Diff revision 4)
> +    return <div>{props.content}</div>;
> +  }
> +  ```
> +  These functions are actually equivalent to classes extending `Component`. In
> +  the rest of the article we'll especially focus on the latter. Unless said 
> +  otherwise everything said about classes extending `Component` is also true for

"said" can be dropped

::: devtools/docs/contributing/react-performance-tips.md:44
(Diff revision 4)
> +  the rest of the article we'll especially focus on the latter. Unless said 
> +  otherwise everything said about classes extending `Component` is also true for
> +  Stateless/Functional Components.
> +
> +#### Notes on the use of JSX
> +Because we don't use any build step in mozilla-central yet, some of our

nit: any -> a 

reason is that "any" supposes that the subject is plural, so build step would have to be build steps

::: devtools/docs/contributing/react-performance-tips.md:59
(Diff revision 4)
> +
> +We'll use JSX in this documentation for more clarity but this is strictly
> +equivalent. You can read more on [React documentation](https://reactjs.org/docs/react-without-jsx.html).
> +
> +### The first render
> +To start a React application and trigger a first render, there's only one way:

This reads a bit strange in english, it might be clearer to move the second half to the front:

There's only one way to start a React application and trigger a first render.

::: devtools/docs/contributing/react-performance-tips.md:91
(Diff revision 4)
> +   trigger updates too.
> +3. One component's props change. But note that this can't happen by itself, this
> +   is always a consequence of the case 1 or 2 in one of its parents. So we'll
> +   ignore this case for this chapter.
> +
> +When one of these happens, React will call that component's `render` method, and then

this looks like it might be a duplication of the information in lines 69 to 72.

Maybe to make the significance claer, you can start with "Like the initial render, when a component rerenders, React will... "

::: devtools/docs/contributing/react-performance-tips.md:114
(Diff revision 4)
> +   authors, which means the processing duration increases linearly with the number
> +   of elements in the tree we compare.
> +
> +Let's dive more into each one of these issues.
> +
> +#### Do not render too often

these headers should somehow be the same as the list above, as it will make it easier to search / scan the document. In this case it would be "triggering the render process too frequently"

Maybe for each point you can have a subheader?

::: devtools/docs/contributing/react-performance-tips.md:154
(Diff revision 4)
> +measurements, and do not call `setState` as this would trigger yet another
> +update.
> +
> +#### Help the reconciliation algorithm be efficient
> +
> +As said earlier, the smaller the tree is, the faster the algorithm is. So it's

As said earlier -> As mentioned above

This is just me being nitpicky though. This part of the sentence can also be safely dropped

::: devtools/docs/contributing/react-performance-tips.md:263
(Diff revision 4)
> +* It's much simpler to think about.
> +* It's much faster to check for equality in `shouldComponentUpdate` and in other
> +  places (like Redux' selectors).
> +
> +### About immutability
> +#### What it doesn't mean

this is great. Maybe we can also introduce eslint configs to enforce this? If we don't have them already?

::: devtools/docs/contributing/react-performance-tips.md:287
(Diff revision 4)
> +
> +```javascript
> +this.setState(state => {
> +  // Please don't do this as this will likely induce bugs.
> +  state.arrayObject.push(newItem);
> +  return state;

the example you have at the bottom is good, i was expecting it to be here though! Maybe it makes sense to give the counter example right after the "wrong" version, rather than in its own section?

::: devtools/docs/contributing/react-performance-tips.md:319
(Diff revision 4)
> +
> +If you're using Redux ([see below as well](#a-few-words-about-redux)) this
> +advice applies to your individual reducers as well, even if Redux' tools makes
> +it easy to have a nested/combined state.
> +
> +#### How to update an array

i think this should be moved up to go with the "dont do this" examples, since the simple objects doesn't add to this section, in terms of comprehension

::: devtools/docs/contributing/react-performance-tips.md:343
(Diff revision 4)
> +  return {
> +    // Because we want to keep the old array if removeElement isn't in the
> +    // filtered array, we compare the lengths.
> +    // We still start a render phase because we call `setState`, but thanks to
> +    // PureComponent's shouldComponentUpdate implementation we won't actually render.
> +    stateArray: newArray.length === stateArray.length ? stateArray : newArray,

this state object will have a different identity even if the subobject is the same. probably you meant 

```javascript
if (newArray.length === stateArray.length) {
  return { stateArray: new Array };
}

return state;
```

::: devtools/docs/contributing/react-performance-tips.md:355
(Diff revision 4)
> +Updating an object is similar. In this example we'll use the object spread
> +operator, already implemented in Firefox, Chrome and Babel.
> +
> +```javascript
> +this.setState(({ stateObject }) => ({
> +  stateObject: stateObject.property === newValue

this will return new state object each time, so if something isnt evaluating the substate for changes, it might rerender. This is the case for both pureComponents and regular components, pure components only check if the state has the same identity

i think what you want here is 

```javascript
this.setState((state) => {
  if (stateObject.property === newValue) {
    return state;
  }
  
  return { ... state, stateObject: { ...stateObject, property: newValue };
    // equivalent to Object.assign({}, stateObject, { property: newValue })
    });
```

And a nit: there is a trailing comma here that would cause a syntax error.

Comment 26

[Yulia Startsev [:yulia] OOO until July 2026]

Looks good! a couple of english nits and a few things regarding the code examples

Comment 27

[Julien Wajsberg [:julienw]]

Thanks Yulia, I'll update the doc following your suggestions.

One thing about the suggestions you make about `setState` though, I think you misunderstood what setState and pure components do.

Here is what the documentation for `setState` says:
* setState() will always lead to a re-render unless shouldComponentUpdate() returns false.
* The output of the updater is shallowly merged with prevState.

=> it doesn't matter whether you return the same state value from the updater, as it's _not_ the state that you'll get, but rather everything happens as if `setState` does `Object.assign(oldState, updaterResult)`. I don't know if it creates a new object, the documentation doesn't say [1]. But that means the result of the function is _never_ the state you'll get.

[1] Actually it looks like they do both depending on some conditions: https://github.com/facebook/react/blob/94518b068bf52196abea0c83f4c9926bfe2065c6/packages/react-reconciler/src/ReactFiberUpdateQueue.js#L315-L320

And for PureComponents, yes it checks for a direct reference equality, but as long as you called `setState` this will never be true. However it also does a shallow check on the state's every properties.

That means that trying to return the state parameter if possible is useless. Rather it could be better to check these conditions before calling `setState` at all, _and_ check again inside `setState` because of the asynchronicity, but I believe that thanks to the shallow equality in `shouldComponentUpdate` it's not worth the complexity.

Comment 28

[Yulia Startsev [:yulia] OOO until July 2026]

yep! you are right! I was using this as a reference:

"A PureComponent will always re-render if the state or props reference a new object. "
https://60devs.com/pure-component-in-react.html

So that is why i thought it would re-render in the case of a pure component.

so, you can disregard that comment :)

Comment 29

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review230684

Comment 30

[Yulia Startsev [:yulia] OOO until July 2026]

I gave an r+, once the english nits are fixed its good to go from my perspective!

Comment 31

[Julien Wajsberg [:julienw]]

> That means that trying to return the state parameter if possible is useless. Rather it could be better to check these conditions before calling `setState` at all, _and_ check again inside `setState` because of the asynchronicity, but I believe that thanks to the shallow equality in `shouldComponentUpdate` it's not worth the complexity.

I realize we could also return `null` to avoid an update. I haven't tried directly but this is what I read from React's source code. I'll try it.

Comment 32

[Julien Wajsberg [:julienw]]

> And a nit: there is a trailing comma here that would cause a syntax error.

I don't think I see the trailing comma you're mentioning. Trailing commas in object only cause syntax errors in very old browsers, I think.

Comment 33

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/4-5/

Comment 34

[Julien Wajsberg [:julienw]]

I updated the page from Yulia's feedback. I also added some lines outlining the thing that got Yulia confused.

Now waiting for the final feedback from Nicolas!

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review231176

Thanks Julian, this is good.
I have a few comments, but nothing too important :)

::: devtools/docs/contributing/react-performance-tips.md:9
(Diff revision 5)
> +well as discuss some tips to make your React application faster.
> +
> +## TL;DR tips
> +
> +* Prefer props and state immutability and use `PureComponent` components as a default
> +* As a convention, the reference should change **if and only if** the inner data

It can be unclear which reference we are talking about

::: devtools/docs/contributing/react-performance-tips.md:11
(Diff revision 5)
> +  * Never use bound or anonymous functions as props to a Component (it's fine to use
> +    them as props to a DOM element).

I guess the issue is more if the function is inlined ?

e.g. those are fine:

```js
const onA = () => {}
const onB = (() => {}).bind(this)

class Y extends React.component {
  render() {
    <Z onA={onA} onB={onB}>
  }
}
```

::: devtools/docs/contributing/react-performance-tips.md:38
(Diff revision 5)
> +  ```jsx
> +  function Application(props) {
> +    return <div>{props.content}</div>;
> +  }
> +  ```
> +  These functions are equivalent to classes extending `Component`. In

I think this is only true for JSX. When Reps switched from components to plain functions returning React elements we say a quite important perf improvement.

And from what I remember reading React code those simple functions are not wrapped into something: they just return a React element.

I think what matter is how you use those functions.

If I take the example: 

```jsx
function Text(props) {
  return <span>{props.content}</span>;
}
```

And use it:
```jsx
class Application extends React.Component {
  render() {
    return <Text>Hello</Text>
  }
}
```

There's no benefit (because jsx gets converted to something like `React.createElement(Text, props)`). But if I simply do a function call:

```js
class Application extends React.Component {
  render() {
    return Text({content: "Hello"});
  }
}
```

React won't do that extra step since the render function already returns a React element. Note that we don't pass `Tect` in `createFactory` as well: it's just a simple function call.

::: devtools/docs/contributing/react-performance-tips.md:43
(Diff revision 5)
> +#### Notes on the use of JSX
> +Because we don't use a build step in mozilla-central yet, some of our
> +tools don't use JSX and use [factories](https://reactjs.org/docs/react-api.html#createfactory)
> +instead:
> +```javascript
> +class Application extends React.Component {
> +  render() {
> +    return dom.div(null, this.props.content);
> +  }
> +}
> +```

nit: Perhaps we could wrap this in `<aside class="notice">` ?

::: devtools/docs/contributing/react-performance-tips.md:123
(Diff revision 5)
> +Everything that's in the state should be used in `render`.
> +Anything in the state that's not used in `render` shouldn't be in the state, but
> +rather in an instance variable. This way you won't trigger an update if you
> +change some internal state that you don't want to reflect in the UI.
> +
> +But if you call `setState` from an event handler you may call it too often.

nit: The `But` doesn't seem to relate to anything ? I guess that's a leftover from a previous iteration. we could remove it imo

::: devtools/docs/contributing/react-performance-tips.md:126
(Diff revision 5)
> +change some internal state that you don't want to reflect in the UI.
> +
> +But if you call `setState` from an event handler you may call it too often.
> +This is usually not a problem because React is smart enough to merge close
> +setState calls and trigger a rerender only once per frame. Yet if your `render`
> +is expensive (see below as well) this could lead to problem and you may want to

s/problem/problems ?

::: devtools/docs/contributing/react-performance-tips.md:129
(Diff revision 5)
> +This is usually not a problem because React is smart enough to merge close
> +setState calls and trigger a rerender only once per frame. Yet if your `render`
> +is expensive (see below as well) this could lead to problem and you may want to
> +use `setTimeout` or other similar techniques to throttle the renders.
> +
> +In addition, try to change the state as close as possible to where your UI

Here I guess you are saying not to pipe down props on too many levels to prevent updating unnecessary "parents". 
Maybe an example would help

::: devtools/docs/contributing/react-performance-tips.md:142
(Diff revision 5)
> +chunks of items or to
> +[virtualize this list](https://reactjs.org/docs/optimizing-performance.html#virtualize-long-lists).
> +Although this is not always possible or easy.
> +
> +Do not do heavy computations in your `render` methods. Rather do them before
> +setting the state, and set the state to the result to these computations.

s/to the result to/as a result of ?

::: devtools/docs/contributing/react-performance-tips.md:163
(Diff revision 5)
> +a lot by skipping parts that likely haven't changed. There are eslint rules to
> +help to check this:
> +[react/jsx-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-key.md),
> +part of the recommended rules of [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react/),
> +and
> +[react/no-array-index-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md).

I'm unsure if we should talk about the eslint rules here. If we decide it's important, let's have them as part of our projects (rfc ? :) )

Here, there's nothing the contributor can do on its own.

::: devtools/docs/contributing/react-performance-tips.md:201
(Diff revision 5)
> +    this.setState({ detailsOpen: true });
> +  }
> +
> +  // Return false to avoid a render
> +  shouldComponentUpdate(nextProps, nextState) {
> +    // Note: this works only if `summary` and `content` aren't complex mutable objects.

nit: not sure about the `complex` here. As long as the props is mutable it's unsafe, no matter the complexity of it.

::: devtools/docs/contributing/react-performance-tips.md:222
(Diff revision 5)
> +}
> +```
> +
> +__This is a very efficient way to improve your application speed__, because this
> +avoids everything: both calling render methods for this component _and_ the
> +whole subtree, and the reconciliation phase for this subtree.

Maybe we could add a reminder that shouldComponentUpdate should be lightweight, and that the use should return as fast as possible (like in your example)

::: devtools/docs/contributing/react-performance-tips.md:253
(Diff revision 5)
> +}
> +```
> +
> +This has a very important consequence: for complex props and states, that is
> +objects and arrays that can be mutated without changing the reference itself,
> +PureComponent's `shouldComponentUpdate` will yield wrong results and will skip

you are talking about PureComponent shouldComponentUpdate, but it isn't shown in the code, and can confuse the reader.

::: devtools/docs/contributing/react-performance-tips.md:322
(Diff revision 5)
> +  stateObject: stateObject.property === newValue
> +    ? stateObject
> +    : { ...stateObject, property: newValue },
> +    // equivalent to Object.assign({}, stateObject, { property: newValue })

nit: could we use `details` instead of `property`, like in the previous examples

::: devtools/docs/contributing/react-performance-tips.md:388
(Diff revision 5)
> +    ? null
> +    : { stateArray: newArray };
> +});
> +```
> +
> +#### How to update primitive values

before primitive, we could talk about Map and Set as well perhaps ?

::: devtools/docs/contributing/react-performance-tips.md:434
(Diff revision 5)
> +    return <MyComponent onUpdate={() => this.update()} />;
> +  }
> +  ```
> +
> +  Each time the `render` method runs, a new function will be created, and the
> +  shallow check will fail.

nit: specify that it's the check of the `MyComponent` component that will fail.

::: devtools/docs/contributing/react-performance-tips.md:435
(Diff revision 5)
> +  There's an [eslint rule](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md)
> +  to check this.

Same as previously, I'm not sure we should discuss eslint rules in this document

::: devtools/docs/contributing/react-performance-tips.md:483
(Diff revision 5)
> +you can especially enable "Highlight Updates" that makes it a lot easier to see
> +which components are updated by React.
> +
> +### Devtools' paint flashing feature
> +In Firefox' Devtools, you can enable the ["paint flashing"
> +feature](https://developer.mozilla.org/en-US/docs/Tools/Paint_Flashing_Tool) from the

i think this is only works for content right ? there's another thing for flashing in chrome, maybe we could talk about it as well.

::: devtools/docs/contributing/react-performance-tips.md:489
(Diff revision 5)
> +devtools' settings. This adds an handy button to paint DOM reflows. This is very
> +handy to debug your performance issues at a low level.
> +
> +### Good ol' `console.log`
> +Instrumenting your code by adding `console.log` statements in `render` methods
> +makes it very easy to linearly follow how your components are updated.

we could even mention `console.count("MyComponent render")` which will give you an incremented counter each time it is called

Comment 36

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review231194

::: devtools/docs/contributing/react-performance-tips.md:14
(Diff revision 5)
> +* As a convention, the reference should change **if and only if** the inner data
> +  changes.
> +  * Never use bound or anonymous functions as props to a Component (it's fine to use
> +    them as props to a DOM element).
> +  * Be careful to not update a reference if the inner data doesn't change.
> +* [Always measure before optimizing](./performance.md) to have a real impact on performance.

s/before// || s/before/while/
in theory you should measure:
* before, to find possible culprits
* after, to confirm your fix has an impact.

::: devtools/docs/contributing/react-performance-tips.md:109
(Diff revision 5)
> +come from:
> +1. triggering the render process **too frequently**,
> +2. **expensive** render methods,
> +3. the reconciliation algorithm itself. The algorithm is O(n) according to React
> +   authors, which means the processing duration increases linearly with **the number
> +   of elements in the tree** we compare.

I see another case that you cover in next paragraph:
render a larger tree than what has been updated.

::: devtools/docs/contributing/react-performance-tips.md:201
(Diff revision 5)
> +    this.setState({ detailsOpen: true });
> +  }
> +
> +  // Return false to avoid a render
> +  shouldComponentUpdate(nextProps, nextState) {
> +    // Note: this works only if `summary` and `content` aren't complex mutable objects.

I imagine you wanted to say "are primitive objects."
this is the way to refer to strings, number, boolean... and everything where two distinct instances of the same value are equal.

::: devtools/docs/contributing/react-performance-tips.md:256
(Diff revision 5)
> +This has a very important consequence: for complex props and states, that is
> +objects and arrays that can be mutated without changing the reference itself,
> +PureComponent's `shouldComponentUpdate` will yield wrong results and will skip
> +renders where it shouldn't. In this case, you'll either need to implement your own
> +`shouldComponentUpdate`… or (__preferred__) decide to make all your data
> +structure immutable. The latter is preferred because:

I think you have to have immutable objects to use PureComponents. Otherwise you should use regular Component with custom shouldComponentUpdate implementation.
So I would rather say something like:
  To use PureComponents, you have to use immutable objects in props and states. Otherwise you should use regular Components with custom shouldComponentUpdate method.

::: devtools/docs/contributing/react-performance-tips.md:289
(Diff revision 5)
> +them not nested, so that you don't need to use a library like
> +[immutability-helper](https://github.com/kolodny/immutability-helper),
> +[updeep](https://github.com/substantial/updeep), or even
> +[Immutable](https://github.com/facebook/immutable-js). Be especially careful
> +with Immutable as it's easy to create performance problems using it by misusing
> +its API.

s/usint it//

::: devtools/docs/contributing/react-performance-tips.md:291
(Diff revision 5)
> +[updeep](https://github.com/substantial/updeep), or even
> +[Immutable](https://github.com/facebook/immutable-js). Be especially careful
> +with Immutable as it's easy to create performance problems using it by misusing
> +its API.
> +
> +Keeping the objects simple makes it easy to update them.

This looks redundant with previous paragraph.

::: devtools/docs/contributing/react-performance-tips.md:294
(Diff revision 5)
> +its API.
> +
> +Keeping the objects simple makes it easy to update them.
> +
> +If you're using Redux ([see below as well](#a-few-words-about-redux)) this
> +advice applies to your individual reducers as well, even if Redux' tools makes

s/'// or s/'/'s/, and s/makes/make/

::: devtools/docs/contributing/react-performance-tips.md:334
(Diff revision 5)
> +// avoid the render cycle at all.
> +this.setState(({ stateObject }) => (
> +  stateObject.property === newValue
> +    ? null
> +    : { ...stateObject, property: newValue }
> +);

I would start with this simplier example first.
(I'm not sure the first one, more complex is really useful to understand this)

::: devtools/docs/contributing/react-performance-tips.md:361
(Diff revision 5)
> +Instead here again you need to **create a new array instance**.
> +
> +```javascript
> +// Adding an element is easy.
> +// Note we use the callback version of setState because we use the state to
> +// build a new state.

You should move this comment in the first setState example as it wasn't clear why you use setState callback everywhere. Until reaching the array example.

::: devtools/docs/contributing/react-performance-tips.md:457
(Diff revision 5)
> +  [memoize-immutable](https://github.com/memoize-immutable/memoize-immutable)
> +  can be useful in some cases too.
> +
> +## React with Redux
> +
> +[TODO](https://bugzilla.mozilla.org/show_bug.cgi?id=1440696)

I'm not sure it is any useful to land such a TODO note in doc page.

::: devtools/docs/contributing/react-performance-tips.md:459
(Diff revision 5)
> +
> +## React with Redux
> +
> +[TODO](https://bugzilla.mozilla.org/show_bug.cgi?id=1440696)
> +
> +## Diagnosing performance issues with some tooling

This whole paragraph looks redundant with performance.md.
Please try to refer to it and contribute to it if something is missing or is unclear.
Do not hesitate to add react specific tooling to performance.md. We use react almost everywhere so it makes sense to talk about react tooling.

Comment 37

[Julien Wajsberg [:julienw]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #35)

Thanks for your thoughful suggestions!
Here are some comments on some. note that I removed most things I agree with :)

> ::: devtools/docs/contributing/react-performance-tips.md:11
> (Diff revision 5)
> > +  * Never use bound or anonymous functions as props to a Component (it's fine to use
> > +    them as props to a DOM element).
> 
> I guess the issue is more if the function is inlined ?
> 
> e.g. those are fine:
> 
> ```js
> const onA = () => {}
> const onB = (() => {}).bind(this)
> 
> class Y extends React.component {
>   render() {
>     <Z onA={onA} onB={onB}>
>   }
> }
> ```

Yeah I believe you're right, I should precise this better.
For example we can also definitely create a named function in the `render` method, and this is still wrong.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:38
> (Diff revision 5)

> I think this is only true for JSX. When Reps switched from components to
> plain functions returning React elements we say a quite important perf
> improvement.
> 
> And from what I remember reading React code those simple functions are not
> wrapped into something: they just return a React element.

I believe you misunderstood how this works (or maybe it changed with React 16?). `createFactory` is just a wrapper for `createElement` [1], just like JSX is converted at buildtime to `createElement` calls. The runtime costs should be identical.

[1] https://github.com/facebook/react/blob/373a33f9d30d02cfea814a7803209da6f860a252/packages/react/src/ReactElement.js#L263-L272

> > +Do not do heavy computations in your `render` methods. Rather do them before
> > +setting the state, and set the state to the result to these computations.
> 
> s/to the result to/as a result of ?

Mmm I think it should be "to the result of" actually. Thanks for pointing this!

> 
> ::: devtools/docs/contributing/react-performance-tips.md:163
> (Diff revision 5)
> > +a lot by skipping parts that likely haven't changed. There are eslint rules to
> > +help to check this:
> > +[react/jsx-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-key.md),
> > +part of the recommended rules of [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react/),
> > +and
> > +[react/no-array-index-key](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md).
> 
> I'm unsure if we should talk about the eslint rules here. If we decide it's
> important, let's have them as part of our projects (rfc ? :) )
> 
> Here, there's nothing the contributor can do on its own.

I wanted to mention them as a good hint at implementing them ;) When we do we can change this part to "we have eslint rules" :D

> 
> ::: devtools/docs/contributing/react-performance-tips.md:201
> (Diff revision 5)
> > +    this.setState({ detailsOpen: true });
> > +  }
> > +
> > +  // Return false to avoid a render
> > +  shouldComponentUpdate(nextProps, nextState) {
> > +    // Note: this works only if `summary` and `content` aren't complex mutable objects.
> 
> nit: not sure about the `complex` here. As long as the props is mutable it's
> unsafe, no matter the complexity of it.

I use the word "complex" as opposite to "primitive". Maybe I should just use "non-primitive"...

Comment 38

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot [:ochameau] from comment #36)

Thanks Alexandre!

A few comments on your suggestions:

> ::: devtools/docs/contributing/react-performance-tips.md:109
> (Diff revision 5)
> > +come from:
> > +1. triggering the render process **too frequently**,
> > +2. **expensive** render methods,
> > +3. the reconciliation algorithm itself. The algorithm is O(n) according to React
> > +   authors, which means the processing duration increases linearly with **the number
> > +   of elements in the tree** we compare.
> 
> I see another case that you cover in next paragraph:
> render a larger tree than what has been updated.

Oh yeah, I put this part inside another part, like an afterthought, but you're right it should have its own part.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:256
> (Diff revision 5)

> I think you have to have immutable objects to use PureComponents. Otherwise
> you should use regular Component with custom shouldComponentUpdate
> implementation.
> So I would rather say something like:
>   To use PureComponents, you have to use immutable objects in props and
> states. Otherwise you should use regular Components with custom
> shouldComponentUpdate method.

So, it's both right and wrong :-)
you can actually implement a shouldComponentUpdate method in a PureComponent, and this will work fine. But it's useless as then it's just like a Component (a PureComponent is nothing more than a Component with a nice shouldComponentUpdate implementation).
I'll try to make this clearer.


> ::: devtools/docs/contributing/react-performance-tips.md:294
> (Diff revision 5)
> > +its API.
> > +
> > +Keeping the objects simple makes it easy to update them.
> > +
> > +If you're using Redux ([see below as well](#a-few-words-about-redux)) this
> > +advice applies to your individual reducers as well, even if Redux' tools makes
> 
> s/'// or s/'/'s/, and s/makes/make/

Thanks, I thought that because Redux ends with a -x (and a -s sound) it shouldn't have the "s". But I checked and you're right :)

> 
> ::: devtools/docs/contributing/react-performance-tips.md:361
> (Diff revision 5)
> > +Instead here again you need to **create a new array instance**.
> > +
> > +```javascript
> > +// Adding an element is easy.
> > +// Note we use the callback version of setState because we use the state to
> > +// build a new state.
> 
> You should move this comment in the first setState example as it wasn't
> clear why you use setState callback everywhere. Until reaching the array
> example.

Ah yeah, the "array" came first initially, I moved around the paragraphs yesterday and forgot about it, thanks !

> 
> ::: devtools/docs/contributing/react-performance-tips.md:457
> (Diff revision 5)
> > +  [memoize-immutable](https://github.com/memoize-immutable/memoize-immutable)
> > +  can be useful in some cases too.
> > +
> > +## React with Redux
> > +
> > +[TODO](https://bugzilla.mozilla.org/show_bug.cgi?id=1440696)
> 
> I'm not sure it is any useful to land such a TODO note in doc page.

It was mostly so that no reviewer tells me "why you don't say anything about redux" :D
Do you think I should just remove this part, or write a clearer sentence?

> 
> ::: devtools/docs/contributing/react-performance-tips.md:459
> (Diff revision 5)
> > +
> > +## React with Redux
> > +
> > +[TODO](https://bugzilla.mozilla.org/show_bug.cgi?id=1440696)
> > +
> > +## Diagnosing performance issues with some tooling
> 
> This whole paragraph looks redundant with performance.md.
> Please try to refer to it and contribute to it if something is missing or is
> unclear.
> Do not hesitate to add react specific tooling to performance.md. We use
> react almost everywhere so it makes sense to talk about react tooling.

OK, I'll look at this closer and ask you a review if I add things to performance.md.

Comment 39

[Alexandre Poirot [:ochameau] PTO, back August 11th]

(In reply to Julien Wajsberg [:julienw] from comment #38)
> > ::: devtools/docs/contributing/react-performance-tips.md:457
> > (Diff revision 5)
> > > +  [memoize-immutable](https://github.com/memoize-immutable/memoize-immutable)
> > > +  can be useful in some cases too.
> > > +
> > > +## React with Redux
> > > +
> > > +[TODO](https://bugzilla.mozilla.org/show_bug.cgi?id=1440696)
> > 
> > I'm not sure it is any useful to land such a TODO note in doc page.
> 
> It was mostly so that no reviewer tells me "why you don't say anything about
> redux" :D
> Do you think I should just remove this part, or write a clearer sentence?

Remove it, there is no value for the reader to see this.

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/5-6/

Comment 41

[Julien Wajsberg [:julienw]]

I pushed the fix for previous comments. Requested a last review from :ochameau especially because I moved things around and added some text to the previously existing performance page.

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/6-7/

Comment 43

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

https://reviewboard.mozilla.org/r/222716/#review237510

Could you proceed with react-performance-tips.md and keep performance.md contribution for a distinct bug?

::: devtools/docs/contributing/react-performance-tips.md:341
(Diff revisions 5 - 6)
> -});
> -
> -// If you have just one property to update, you can also return a falsy value to
>  // avoid the render cycle at all.
> -this.setState(({ stateObject }) => (
> -  stateObject.property === newValue
> +this.setState(({ stateObject }) => {
> +  // Note we compare inside the callback because the comparison

Note we => Note that we?

::: devtools/docs/contributing/react-performance-tips.md:348
(Diff revisions 5 - 6)
> -    : { ...stateObject, property: newValue }
> +  if (stateObject.details === newValue) {
> +    // This avoids the render cycle altogether.
> +    return null;
> +  }
> +
> +  // returns a new object for `stateObject`

// Returns a copy of `stateObject` with updated value for `details`

::: devtools/docs/contributing/react-performance-tips.md:356
(Diff revisions 5 - 6)
> +    // equivalent to Object.assign({}, stateObject, { details: newValue })
> +  };
>  );
> +
> +// If you want to update more than one property, it's easier to rely on the
> +// cheap comparison in `shouldComponentUpdate`.

cheap comparison:
  short syntax
  ternary operators
  conditional operators

::: devtools/docs/contributing/react-performance-tips.md:358
(Diff revisions 5 - 6)
>  );
> +
> +// If you want to update more than one property, it's easier to rely on the
> +// cheap comparison in `shouldComponentUpdate`.
> +this.setState(({ stateObject1, stateObject2 }) => ({
> +  stateObject1: stateObject1.content === newContenta

s/newContenta/newtContent/

::: devtools/docs/contributing/performance.md:159
(Diff revision 6)
> +## Diagnosing performance issues in React-based applications
> +
> +A lot of our devtools is now written using the framework [React](http://reactjs.org/).
> +You can follow some
> +[React-specific code recommendation](./react-performance-tips.md) to get a good
> +baseline performance, but even after following these tips the performance of

baseline +on or about+ performance?

::: devtools/docs/contributing/performance.md:170
(Diff revision 6)
> +* in the call tree, which functions take the most time;
> +* in the markers, which components are updated as the result of an action.
> +  Indeed, React (Dev edition) inserts performance markers for this process so
> +  that you can more easily analyze what happens.
> +
> +  ![React markers](performance/profiler-react-markers.png)

I don't get any react marker by default,
I imagine we need to debug flag in mozconfig?
We should document that.
The main goal of this doc is to document contributiong made against mozilla-central, not perf-html, nor debugger.html via launchpad.

::: devtools/docs/contributing/performance.md:185
(Diff revision 6)
> +### Using Devtools' paint flashing feature
> +For non-privileged pages running in content, for example the debugger or the
> +console run from the dashboard, you can enable the ["paint flashing"
> +feature](https://developer.mozilla.org/en-US/docs/Tools/Paint_Flashing_Tool) from the
> +devtools' settings. This adds an handy button to paint DOM reflows. This is very
> +handy to debug your performance issues at a low level.

I would skip this paragraph as well as react add-on as one as this isn't covering the main contribution workflow.

::: devtools/docs/contributing/performance.md:192
(Diff revision 6)
> +For Chrome and privileged code, there's a similar feature that you can enable
> +with a preference in `about:config`, the preference is called
> +`nglayout.debug.paint_flashing_chrome`.
> +
> +With this tool you can ensure that only a minimal surface is being repainted
> +for some UI update.

You may paste the gif from https://docs.google.com/document/d/1LiLFCiC32kjZ9_JDfDOE37sKFLEs6n9F9sIQPQuzWu8/edit#
to show how this feature works.

::: devtools/docs/contributing/performance.md:194
(Diff revision 6)
> +`nglayout.debug.paint_flashing_chrome`.
> +
> +With this tool you can ensure that only a minimal surface is being repainted
> +for some UI update.
> +
> +### Using the good ol' `console.log` and `console.count`

s/ol'/old/

::: devtools/docs/contributing/performance.md:199
(Diff revision 6)
> +### Using the good ol' `console.log` and `console.count`
> +Instrumenting your code by adding `console.log` statements in `render` methods
> +makes it very easy to linearly follow how your components are updated.
> +
> +`console.count` is handy too, as it counts how many times it's been called at
> +this specific line.

s/this/a/ ?

Comment 44

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot [:ochameau] from comment #43)
> Comment on attachment 8953479 [details]
> Bug 1435991 - Document React component best usage
> 
> https://reviewboard.mozilla.org/r/222716/#review237510
> 
> Could you proceed with react-performance-tips.md and keep performance.md
> contribution for a distinct bug?
> 
> ::: devtools/docs/contributing/react-performance-tips.md:341
> (Diff revisions 5 - 6)
> > -});
> > -
> > -// If you have just one property to update, you can also return a falsy value to
> >  // avoid the render cycle at all.
> > -this.setState(({ stateObject }) => (
> > -  stateObject.property === newValue
> > +this.setState(({ stateObject }) => {
> > +  // Note we compare inside the callback because the comparison
> 
> Note we => Note that we?

The sentence itself isn't nice, I'll rewrite it.

> 
> ::: devtools/docs/contributing/react-performance-tips.md:356
> (Diff revisions 5 - 6)
> > +    // equivalent to Object.assign({}, stateObject, { details: newValue })
> > +  };
> >  );
> > +
> > +// If you want to update more than one property, it's easier to rely on the
> > +// cheap comparison in `shouldComponentUpdate`.
> 
> cheap comparison:
>   short syntax
>   ternary operators
>   conditional operators

Especially here, this is PureComponent's shouldComponentUpdate, so I'm more stating the fact that we know it's cheap.
But I can write what you propose in a previous paragraph where I state "So it should execute some cheap comparisons only."

> 
> ::: devtools/docs/contributing/performance.md:159
> (Diff revision 6)
> > +## Diagnosing performance issues in React-based applications
> > +
> > +A lot of our devtools is now written using the framework [React](http://reactjs.org/).
> > +You can follow some
> > +[React-specific code recommendation](./react-performance-tips.md) to get a good
> > +baseline performance, but even after following these tips the performance of
> 
> baseline +on or about+ performance?

"baseline" can actually be used as an adjective too :)

> 
> ::: devtools/docs/contributing/performance.md:170
> (Diff revision 6)
> > +* in the call tree, which functions take the most time;
> > +* in the markers, which components are updated as the result of an action.
> > +  Indeed, React (Dev edition) inserts performance markers for this process so
> > +  that you can more easily analyze what happens.
> > +
> > +  ![React markers](performance/profiler-react-markers.png)
> 
> I don't get any react marker by default,
> I imagine we need to debug flag in mozconfig?
> We should document that.

Yeah, I write it's the "dev edition". We should accordingly document how to get it (which is with "--enable-debug-js-modules").

> The main goal of this doc is to document contributiong made against
> mozilla-central, not perf-html, nor debugger.html via launchpad.

I could argue that using the launchpad could help debugging too. But I agree this isn't the main flow.

> 
> ::: devtools/docs/contributing/performance.md:185
> (Diff revision 6)
> > +### Using Devtools' paint flashing feature
> > +For non-privileged pages running in content, for example the debugger or the
> > +console run from the dashboard, you can enable the ["paint flashing"
> > +feature](https://developer.mozilla.org/en-US/docs/Tools/Paint_Flashing_Tool) from the
> > +devtools' settings. This adds an handy button to paint DOM reflows. This is very
> > +handy to debug your performance issues at a low level.
> 
> I would skip this paragraph as well as react add-on as one as this isn't
> covering the main contribution workflow.

I could at least make the other paragraph the main one and add this as a smaller note.

> 
> ::: devtools/docs/contributing/performance.md:194
> (Diff revision 6)
> > +`nglayout.debug.paint_flashing_chrome`.
> > +
> > +With this tool you can ensure that only a minimal surface is being repainted
> > +for some UI update.
> > +
> > +### Using the good ol' `console.log` and `console.count`
> 
> s/ol'/old/

I was using "ol'" as as sign of affection for this venerable tool, see https://en.wiktionary.org/wiki/ol%27 :)
If that's too much, I can actually use the word "venerable".

Comment 45

[Julien Wajsberg [:julienw]]

> Could you proceed with react-performance-tips.md and keep performance.md contribution for a distinct bug?

Filed Bug 1449602

Comment 46

[Alexandre Poirot [:ochameau] PTO, back August 11th]

(In reply to Julien Wajsberg [:julienw] from comment #44)
> > 
> > ::: devtools/docs/contributing/react-performance-tips.md:356
> > (Diff revisions 5 - 6)
> > > +    // equivalent to Object.assign({}, stateObject, { details: newValue })
> > > +  };
> > >  );
> > > +
> > > +// If you want to update more than one property, it's easier to rely on the
> > > +// cheap comparison in `shouldComponentUpdate`.
> > 
> > cheap comparison:
> >   short syntax
> >   ternary operators
> >   conditional operators
> 
> Especially here, this is PureComponent's shouldComponentUpdate, so I'm more
> stating the fact that we know it's cheap.
> But I can write what you propose in a previous paragraph where I state "So
> it should execute some cheap comparisons only."

Then I don't follow this comment. This paragraph isn't about shouldComponentUpdate.
It doesn't seem related to the setState displayed right after.

> > baseline +on or about+ performance?
> 
> "baseline" can actually be used as an adjective too :)

I read this too quickly...

> > s/ol'/old/
> 
> I was using "ol'" as as sign of affection for this venerable tool, see
> https://en.wiktionary.org/wiki/ol%27 :)
> If that's too much, I can actually use the word "venerable".

I'm mostly thinking about using simple vocabulary here.
To help people starting with english to read our doc that is not translated.

Comment 47

[Julien Wajsberg [:julienw]]

Comment on attachment 8953479 [details]
Bug 1435991 - Document React component best usage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222716/diff/7-8/

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bfeed76aa64
Document React component best usage r=gregtatum,nchevobbe,ochameau,yulia

Comment 49

[Julien Wajsberg [:julienw]]

> Then I don't follow this comment. This paragraph isn't about shouldComponentUpdate.
> It doesn't seem related to the setState displayed right after.

Yes because setState triggers a render cycle. At the start of a render cycle `shouldComponentUpdate` is called to decide whether the render cycle should move forward or just stop here.

So here, the choice is:
* return "null" in setState's callback => this won't trigger a render cycle
* return an object that will be merged into the existing state to create a new state => this triggers a render cycle, that maybe will be stopped by the shouldComponentUpdate call.

As we discussed IRL we agreed on just removing the part about "null". I moved forward and landed the doc as it is now, but I can adjust this part in a follow-up if you think this is still unclear.

Comment 50

[Julien Wajsberg [:julienw]]

> I'm mostly thinking about using simple vocabulary here.
> To help people starting with english to read our doc that is not translated.

Yes I understand. I'll change this in bug 1449602 to something less colloquial.

Comment 51

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1bfeed76aa64