Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Memoize functional components by default #108

Closed
wants to merge 4 commits into from

Conversation

cevr
Copy link

@cevr cevr commented Feb 25, 2019

  • Make functional components implement React.memo by default
  • Add React.always that flags the functional component to be treated as it is currently
  • Add a warning to React.memo when it is used without defining arePropsEqual

View Rendered Text

@theKashey
Copy link

This is not a breaking change. Nothing would break. Just would work not as expected, and it would be almost impossible to fix it.

@cevr
Copy link
Author

cevr commented Feb 25, 2019

This is not a breaking change. Nothing would break. Just would work not as expected, and it would be almost impossible to fix it.

@theKashey Could you elaborate?

@theKashey
Copy link

I'm just remembering how many times something went wrong when I used PureComponents here and there. Event propagation got stopped. Old context, which is still here, broken. Something, what was working yesterday, is no longer working.
PureComponents and memo are not only powerful and useful tools - they are dangerous tools. Cutting an update is a side effect, and sometimes you don't have rights to apply it. It's more about - not every component could understand it's position and a role in the composition.

And it becomes twice more dangerous with a custom arePropsEqual.

@decodejacques
Copy link

In Vue components are pure by default. This is even one of their selling points: "In React, when a component’s state changes, it triggers the re-render of the entire component sub-tree, starting at that component as root." (https://vuejs.org/v2/guide/comparison.html)

This proposed change would break impure components (components that have side effects). Based on the code I've seen, they seem to be the exception. Reasoning about when a component is going to render is something that seems really, really hard.

An example of an impure component: a component that displays the number of times it's been rendered.

Another example: a component that displays the current whether every time it is rendered.

However, in both cases, you could add a prop to the component that gets incremented when it needs to be rerendered or you could explicitly set the shouldComponentUpdate.

I have trouble imagining a useful impure component but I'd like to see an example.

@theKashey
Copy link

theKashey commented Feb 26, 2019

Render props? I dont remember exact case (somewhere in a twitter lands), but it short:

const {variable} = this.state;
return (
 <Layer1>
   <Layer2>
     <Layer3>
       <Something>
         { () => variable } // why my variable is not updated? In which function scope it exists?
      </Something>
    </Layer3> 
   </Layer3>
 </Layer3>  
)

See: facebook/react#12185 (comment)

And it becomes twice more dangerous with a custom arePropsEqual.

Thus said - NonPureComponents are not bad. PureComponents could cut the update, so they could stand next to the update source, and react to the changes (or falsy changes). Behind them, where the update could not occur - there is no need for PureComponents, except some hard optimization cases.

@cevr
Copy link
Author

cevr commented Feb 26, 2019

@theKashey I would argue that examples like that are far from the norm in React. Users who would be using complex patterns such as that would probably recognize whether or not they need a pure or impure component. Providing a React.always would solve that problem. The motivation for this change is that I believe the majority of users would benefit from a functional component being memoized by default, especially with the introduction of hooks.

@theKashey
Copy link

We are all good developers at day, and even better at night. Your proposal could break the code, I was sure about just yesterday.

The problem is - it would not surface the issues my code had, and help me make an application better - it would put it into undefined behavior and leave me alone with untraceable bugs.

It should be opt-in, like <StrictMode>, or provide some reverce debug information like detecting changes in the React.Tree it would cut off, and helping me to find the reasons behind.

@danielkcz
Copy link

danielkcz commented Mar 1, 2019

Just my two cents. Two production apps and we don't use memo at all 😎 React is fast on its own. If the day comes when some noticeable performance drop occurs, then it's easy to try memo as a first band-aid. However, I am convinced it's really a premature optimization that can kill a mood very quickly when something stops updating and it's hard to debug what and where.

Besides, if everything would be memoized by default, you would have to inspect every component on the path, possibly throwing some always along to see if it fixes something. It kinda reminds me console.log debugging style 🤕 I mean it's really hard and I am not aware of proper tools that would allow spotting these issues quickly. Sometimes it's even harder to reproduce those issues that popped in a production to actually attempt fixing them.

Honestly, why would anyone want to introduce possible bugs into their code in exchange for some theoretical optimization? Proper optimization is hard and should be considered as a next step in the development instead of expecting some magic wand to do it for me.

@theKashey
Copy link

But, I liked my idea to have a <PureMode> component, to explicitly apply forced memoization behavior for all the components below it.

@sebmarkbage
Copy link
Collaborator

Originally, the reason React didn't do this is because the principle in place was that ideally it should work but be slower if you did things like mutate something. However, in practice we've found a lot of patterns and cases that just break if you mutate. (With new features concurrent mode, streaming rendering and suspense, this risk significantly increases.)

The funny thing is that React already does this, except a level shallower than memo. React already bails out by default if oldProps === newProps. I.e. if the props object itself is the same. This is ofc much less common to match but it's also significantly cheaper to do.

Looping over properties in a JavaScript object, extracting their value and comparing them isn't free. It adds some cost.

However, there is also a cost associated with React memoizing the result. We have to keep some significant bookkeeping in React to do so. Not just the old and new value but we also keep things like the effects computed by the subtree.

When people talk about other systems doing this automatically, they often don't really mean memo. It's not memoizing so much as keeping track of dependencies to mutable values so it knows what must rerender when those changes. That's what lets small changes make updates deep in a tree.

Another approach is also to have a compiler compute more of the dependencies for you.

I think that's more likely what we'd do here. If we turned on auto memoization it would be more of a heuristic and it wouldn't use the naive dynamic comparison but instead let a component compile to an optimized form of itself that knows which prop changes to look for. In that world, the use of React.memo is still a useful hint to the compiler that you really want this particular component to be memoized.

@NE-SmallTown
Copy link

When people talk about other systems doing this automatically, they often don't really mean memo. It's not memoizing so much as keeping track of dependencies to mutable values so it knows what must rerender when those changes. That's what lets small changes make updates deep in a tree.

@sebmarkbage Could you give more details about this?

@mh-alahdadian
Copy link

I had a one more step idea, when react getted a config for always pure components not only it could handle of props but also it can handle changes of state in useState which can not be handled now anyway

with getting a config for change this default reaction there would be many performance optimizations without any breaking changes

@gaearon
Copy link
Member

gaearon commented Feb 10, 2020

Per #108 (comment), we're not going to do this.

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

Successfully merging this pull request may close these issues.

9 participants