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

React.memo() #63

Merged
merged 8 commits into from Oct 23, 2018

Conversation

Projects
None yet
@gaearon
Member

gaearon commented Oct 19, 2018

View formatted RFC

Note: I just wrote up the RFC. The semantics are designed by @sebmarkbage and @acdlite.

@sag1v

This comment has been minimized.

sag1v commented Oct 19, 2018

This is a great idea, I do think the name equal may confuse some people.
Maybe something closer to the life-cycle methods we have?
shouldUpdate

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

The argument name doesn't matter, it's not technically a part of the API. We can call it arePropsEqual in the docs.

@maciejsikora

This comment has been minimized.

maciejsikora commented Oct 19, 2018

I very have the same doubt about naming of the second function argument equals is not familiar name in React domain. I think shouldUpdate as @sag1v proposed is more accurate, as this function does the same what shouldComponentUpdate in orginal PureComponent

@maciejsikora

This comment has been minimized.

maciejsikora commented Oct 19, 2018

@gaearon the name also will be shown in TypeScript autocomplete features

@baddox

This comment has been minimized.

baddox commented Oct 19, 2018

Note that this is the same API as recompose/pure (except for the optional second argument, although recompose has several other functions to provide similar functionality). I can’t comment much on implementation, but it’s hard to imagine a more clear or convenient API.

I use recompose heavily, declaring all my components as functions and adding state, lifecycle methods, etc. with recompose’s HOCs. I effectively treat this pattern as the missing unification of class and functional components, by never defining class components or working with them directly. It works remarkably well, and could be a great direction for React. React already sneaks in concepts from functional programming, and this would be a big escalation of that wonderful trend. :)

https://github.com/acdlite/recompose/blob/master/docs/API.md#pure

@EyMaddis

This comment has been minimized.

EyMaddis commented Oct 19, 2018

You defined the second parameter to only be a function, I would personally choose an object as it is more future proof and allows for more customization later on, eg. implementing life cycle methods that change props here.

Just as an example, I do not want to derail the conversation:

  equals: lodash.equals,
  lifeCycleMethod1() {
     ...
   }
} 

However using an object does come with a small performance decrease.

PS: Silly me always assumed that function where pure by default.

@streamich

This comment has been minimized.

streamich commented Oct 19, 2018

What if component always stays the same, it would simply have a .shouldComponentUpdate attribute, which—if not implemented—results in current behaviour.

Non-pure component:

const Message = () => <div>Hello world</div>;

Pure component:

const Message = () => <div>Hello world</div>;

Message.shouldComponentUpdate = React.notShallowEqual;

React.pure helper:

React.pure = (component, comparator = React.notShallowEqual) => {
  component.shouldComponentUpdate = comparator;
};
@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

What if component always stays the same, it would simply have a .shouldComponentUpdate prop, which—if not implemented—results in current behaviour.

This introduces an extra runtime property check for every single function component, making the whole app slower and potentially causing deoptimizations. This design was considered (see "Alternatives") and rejected.

You defined the second parameter to only be a function, I would personally choose an object as it is more future proof and allows for more customization later on, eg. implementing life cycle methods that change props here.

There are no plans for making this function more advanced as it would defeat the optimizations it's supposed to provide.

@foiseworth

This comment has been minimized.

foiseworth commented Oct 19, 2018

For clarification, is this RFC about the addition of the second optional argument? I thought this PR meant that in a future release we would have a pure function: https://github.com/facebook/react/pull/13748/files

@baddox

This comment has been minimized.

baddox commented Oct 19, 2018

The shouldComponentUpdate property also mutates the functional component, which means couldn’t easily export the original component and a pure version.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

I thought this PR meant that in a future release we would have a pure function:

This is an RFC covering that PR. We've merged it to test it at FB but now we're getting closer to an open source release and want to get community feedback.

@falconmick

This comment has been minimized.

falconmick commented Oct 19, 2018

I would vote for the breaking change. I would even almost like this to be the case for Component (i.e. get rid of PureComponent and just make shallow compare the default) and instead of making code which works well when mutable, make it so that if you mutate props or state that you get bugs right away instead of in 3 months time when the code has solidified.

This would really suck for new devs, but could help drive home the idea of immutability and pure funcs. oh and good luck with migrating the millions of facebook Components, I don't think a codemod would fix immutability bugs.

Edit: I really don't think this could or would happen, it's more I wish that React tried to push for immutability more

@tomhicks

This comment has been minimized.

tomhicks commented Oct 19, 2018

I think the second argument should be the ”same” as shouldComponentUpdate in that it returns true if it should update.i know the arguments are different for this than a class (nextProps, nextState vs prevProps, nextProps) but returning true when it should update is kind of a React idiom.

To do the opposite (presumably because you just want to put shallowEqual in there) is weird, IMO.

Unless I'm missing something...

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

I would vote for the breaking change. I would even almost like this to be the case for Component (i.e. get rid of PureComponent and just make shallow compare the default) and instead of making code which works well when mutable, make it so that if you mutate props or state that you get bugs right away instead of in 3 months time when the code has solidified.

It wouldn't be just bad because it's a breaking change. It would also be worse because:

  1. Many components will never have shallow bailouts, meaning they would overall make the app slower
  2. It would be impossible to use any regular function component with mutable data coming from above, effectively causing lots of people to convert them to classes (and also make their apps slower)
@Nimelrian

This comment has been minimized.

Nimelrian commented Oct 19, 2018

I would vote for the breaking change. I would even almost like this to be the case for Component (i.e. get rid of PureComponent and just make shallow compare the default)

This would introduce a hefty performance fine in many cases

This would really suck for new devs, but could help drive home the idea of immutability and pure funcs.

I'm a fan of "learn by pain", but the downsides are way too big to introduce this breaking change.

Your comment regarding the internal Facebook components actually makes me think you're a troll.

@streamich

This comment has been minimized.

streamich commented Oct 19, 2018

Would be nice if the second argument was called shouldUpdate instead of arePropsEqual.

  • More consistent with prior art.
  • Maybe your decision to update (or not) the component is not purely based on props being equal (or not equal, or partially equal, or not partially equal).
@tomhicks

This comment has been minimized.

tomhicks commented Oct 19, 2018

@Nimelrian almost certainly he's not trolling. He's saying, if you did go with the breaking change, good luck updating. It's a bit of happy sarcasm that I think you've misinterpreted.

@inferusvv

This comment has been minimized.

inferusvv commented Oct 19, 2018

Why arePropsEqual? In Component we use shouldComponentUpdate which will re-render component if function returns true . And arePropsEqual should return false to re-render component. I think it is a bit confusing and better keep shouldComponentUpdate behavior everywhere.

@falconmick

This comment has been minimized.

falconmick commented Oct 19, 2018

@Nimelrian no not a troll. I used facebook as an example of how this wouldn't work because I am well aware that given their amount of components that they would struggle to support this as their usual strategy for breaking changes are codemods.

It's unrealistic to expect facebook or even more so, the world to be able to handle such a drastic change like that without way too much pain.

tbh my comment was pointless given it's just not feasible to do something as drastic as that, so I really shouldn't have left any more feedback other than I would prefer the default behaviour to be shallow compare. However it's just not something that i see actually having enough net positive.

@tomhicks

This comment has been minimized.

tomhicks commented Oct 19, 2018

Ok I'm glad I'm not the only one thinking it should be shouldUpdate not are same.

I'll go further and say what I wanted to say originally now I know I'm not crazy - unless there's some particular technical objection to it, it's an absolute no-brainer that it should have shouldUpdate behaviour

@sag1v

This comment has been minimized.

sag1v commented Oct 19, 2018

What i love the most about react's naming stuff in API is that its telling us what is the Purpose of the function / parameter, rather than what it Does (implementation details?).
Therefor shouldComponentUpdate or shouldUpdate are a great fit.

Same goes for pure() IMO, this is a pure function component.
Rather then memoized(), this function is memoized (do i need to know this detail?)

@Nimelrian

This comment has been minimized.

Nimelrian commented Oct 19, 2018

@falconmick Than I misread your comment, sorry!

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

This API seems like it enables making third-party components pure. What would be the behavior if you passed an already pure component to pure?

// I don't know if Button is already using React.pure
import {Button} from 'third-party'
import {pure} from 'react'

// Noop? Error?
export default pure(Button);

Following that thread, what happens if you pass a class component? Can users safely apply pure to third party components without knowing how they're implemented?

@jamby77

This comment has been minimized.

jamby77 commented Oct 19, 2018

How about pureComponent(Component, shouldUpdate)

@BurntCaramel

This comment has been minimized.

BurntCaramel commented Oct 19, 2018

This looks great! What would be current advice of when to use this and when not to? Is this generally a good default or often preoptimization?

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

I think memoize or even purify are good candidates.

Argument against memoize: it does not do what the well-known memoize function does. If you import memoize from Lodash in the same file it won't work. That's pretty confusing.

Argument against purify: it's even further from truth than pure. At least with pure we were "tagging" a component as being "pure" in React sense (which was already confusing because it doesn't match the computer science definition of purity). With purify it's as if you can take something impure, and then this function makes it pure. That's not at all what it does.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

With an optional compare property. One would assume that it the same amount of overhead of checking a functions property vs an object property.

We are already checking $$typeof on every type. But if it's a function component we don't need to do more checks. Whereas if we used a flag, we would have to check every single function component.

My main concern and HOC pattern in general is that they can potentially obfuscate the stacktrace by returning a different class/function from a different part of the software stack.

I'm struggling to see how this concern is relevant here, maybe you can provide an example?

@aweary aweary referenced this pull request Oct 19, 2018

Merged

React.lazy() #64

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

Using memo instead of pure also makes the shouldComponentUpdate argument moot because the API is no longer a direct parallel to PureComponent. It makes much more sense to pass an isEqual function to a memoization utility.

@ConAntonakos

This comment has been minimized.

Contributor

ConAntonakos commented Oct 19, 2018

Argument against memoize: it does not do what the memoize function does. If you import memoize from Lodash in the same file it won't work. That's pretty confusing.

Ah, yes, naming conflicts. That makes sense. 👍

Argument against purify: it's even further from truth than pure. At least with pure we were "tagging" a component as being "pure" in React sense (which was already confusing because it doesn't match the computer science definition of purity). With purify it's as if you can take something impure, and then this function makes it pure. That's not at all what it does.

That makes sense as well. 👍 Thank you for explaining!

This is quite the challenge. 😄

`React.memo` returns a component type that tells React "render the inner type, but bail out on updates if props are shallowly equal". In other words, the rendering result is memoized. The prop comparison function can be specified as a second argument to `React.memo`.
`React.memo` only accepts function components as the first argument. It doesn't accept classes since those can extend `PureComponent` directly, and mixing the two approaches is confusing. `React.memo` is intended to be applied at the definition site.

This comment has been minimized.

@aweary

aweary Oct 19, 2018

Member

React.memo only accepts function components as the first argument.

How does this work with React.forwardRef?

This comment has been minimized.

@aweary
@theKashey

This comment has been minimized.

theKashey commented Oct 19, 2018

Would this memo work on the server side between different renders, as real memoization could? (like this)
Sounds like - no, it wouldn't.

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

@theKashey memoizing between requests could run the risk of caching and leaking stale user data.

@tomhicks

This comment has been minimized.

tomhicks commented Oct 19, 2018

@aweary not if it's pure. That's basically the entire point of memorisation...

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

@tomhicks It's still possible if a component in the cached subtree reads data from an external source. memo doesn't guarantee that every descendant component is actually pure. Context can also complicated this.

Most server-rendered apps do a full render at the top level anyways, which makes this a moot point. The API would have to also consider other aspects of caching (where does cached data get stored?), which seems outside the scope of this RFC.

@einarq

This comment has been minimized.

einarq commented Oct 19, 2018

How does it compare performance-wise to wrapping SFC in a PureComponent? Is it using PureComponent under the hood or is it more optimized?

@tomhicks

This comment has been minimized.

tomhicks commented Oct 19, 2018

@aweary in that case it's not pure.

I realise that this is probably academic and your real-wotld example is probably more helpful, but just sayin'

@NE-SmallTown

This comment has been minimized.

NE-SmallTown commented Oct 20, 2018

Argument against memoize: it does not do what the well-known memoize function does.

If so, the memo name is wrong because it still indicates it is the same with 'memoize', just like you want to distinguish between 'Dan' and 'Dan Abramov'

If you import memoize from Lodash in the same file it won't work. That's pretty confusing.

Because any 3rd party library could use the name memoize, so in this situation, we should import * as lodash from 'lodash' rather than don't use the momoize as the name.

@fardarter

This comment has been minimized.

fardarter commented Oct 20, 2018

My request is that code example include how to wrap around a redux-connected component or how to compose with other HOC libraries.

I'd say being able to pass an equality function down is great but it would be great if the code examples included an implementation example.

Also, would be great if the examples showed an example of where shallow equality will fail.

arePropsEqual could be renderOnDifference.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 20, 2018

If so, the memo name is wrong because it still indicates it is the same with 'memoize', just like you want to distinguish between 'Dan' and 'Dan Abramov'

This is fair but you can also think it's short for "memoizing component" or something like this. In practice I don't think it's a problem.

Take "props". It also has a completely different meaning in English. :-) React uses it as short for "properties". Or take "render". React doesn't actually render during the "render" function!

So I understand why it might seem a bit odd at first. But another important consideration is to make it feel "light". This is an optimization API, and it shouldn't feel like a hassle to type. It's weird but that's how people think about APIs. Shorter words "feel" lighter which is what we want to express here.

So we think "memo" will work fine.

@maciejsikora

This comment has been minimized.

maciejsikora commented Oct 22, 2018

I feel changing the name to memo is very good approach, but what problem I see is that from now we will have two the same purpose concepts - PureComponent and memo, where their naming give a feeling that we deal with totally different things. This can be very misleading. Following this, my question is - does React team have a plan to change PureComponent name ( MemoComponent ... oh dont thing so ) or totally deprecate it, as I don't see a place for extending if we can compose by memo HOC.

@reactjs reactjs deleted a comment from maciejsikora Oct 22, 2018

@reactjs reactjs deleted a comment from maciejsikora Oct 22, 2018

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 23, 2018

Following this, my question is - does React team have a plan to change PureComponent name ( MemoComponent ... oh dont thing so ) or totally deprecate it, as I don't see a place for extending if we can compose by memo HOC.

Not immediately but it's plausible that in the longer term memo will be the primary way to do this.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 23, 2018

We decided to allow to pass any component type to memo, and not just functions. Without this, the composition of memo, lazy, and/or forwardRef seemed very confusing.

@gaearon gaearon merged commit cc077c4 into master Oct 23, 2018

@gaearon gaearon deleted the gaearon-patch-1 branch Oct 23, 2018

@acdlite acdlite referenced this pull request Oct 23, 2018

Merged

pure #13748

@gaearon gaearon restored the gaearon-patch-1 branch Oct 24, 2018

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