omniscientjs / omniscient

A library providing an abstraction for React components that allows for fast top-down rendering embracing immutable data for js
http://omniscientjs.github.io/
1.22k stars 51 forks source link

Clarify that shouldComponentUpdate() ignores props.children #97

Closed jabjab closed 9 years ago

jabjab commented 9 years ago

This was mentioned in passing in https://github.com/omniscientjs/omniscient/issues/76, but the fact that shouldComponentUpdate ignores props.children results in omniscient components created with children supplied from their owners not updating when the children change but all other props passed directly to the parent component remain the same. The behavior does make sense from an efficiency standpoint as it's somewhat of an edge case (it won't happen with approaches where every component is tailored to its location in the component hierarchy and/or where the cursor hierarchy has a roughly one-to-one correspondence with the components') and trying to support it generically would potentially mean having to call each child's shouldComponentUpdate from the parent. As well, if absolutely necessary shouldComponentUpdate could be overridden/modified to implement the necessary checks (and if the component is a simple wrapper it could probably be converted to a regular React component with minimal performance impact due to React's reconciliation algorithms or even just override shouldComponentUpdate to always return true as mentioned in https://github.com/omniscientjs/omniscient#cursors).

That being said, while the Cursors section of the readme mentions the issue in passing and provides a possible solution for the simple-wrapper case as mentioned above, it does it in a somewhat confusing way as children are still non-cursor properties of a component; some clarification may therefore be useful.

mikaelbr commented 9 years ago

Thanks for letting us know! I'm sure we can be much clearer on many things. I updated the readme to be more explicit on this.