Workiva / react-dart

Dart Bindings for React JS
BSD 2-Clause "Simplified" License
412 stars 67 forks source link

React.PureComponent Implementation #234

Closed aaronlademann-wf closed 4 years ago

aaronlademann-wf commented 4 years ago

Playing around with this based on Dave Doty's question in the gitter chat...

Thoughts on this approach? The tests seem to confirm behavior that aligns with the JS component, but should we interop the JS React.PureComponent instead?

The only difference is... is the shallow comparison of children a good idea? Or should children always result in updates just like the JS version? We could even disallow children in the component using propTypes...

@greglittlefield-wf @kealjones-wk @joebingham-wk

dave-doty commented 4 years ago

@aaronlademann-wf I assume your discussion about children is related to this comment from the React docs: Furthermore, React.PureComponent’s shouldComponentUpdate() skips prop updates for the whole component subtree. Make sure all the children components are also “pure”.

For what it's worth, I would be in favor of two implementations (or a way to toggle between them), one that assumes the children are pure and skips updating their props if the parent props didn't change, and another version that updates their props to be on the safe side (in case a re-render is needed for views that depend on more than props and state).

I assume that the following is true of the latter case: if the children are also pure and also implementing PureComponent, then they won't ultimately be re-rendered either, but unnecessary computation will be done by calling their shouldComponentUpdate methods (unnecessary because, since they are pure, they will all return false).

The difference in "error modes" is that the former could skip necessary re-renders, resulting in bugs, whereas the latter could call children props comparison code unnecessarily, resulting in slowdowns. Although I tend to favor something that's more likely to be correct, even if slow, it's also going to throw people to have a default that differs from the ReactJS implementation.

I also have to imagine it would be rare for someone to use a PureComponent with non-pure children. (Now watch me come up with a use case for it in my own project next.)