Skip to content

Commit

Permalink
added pure option
Browse files Browse the repository at this point in the history
- only re-render component if props or requests shallowly changed
- this.setAtomicState now creates new objects for all 4 keys
- added tests for `pure: true` and `pure: false`
- added `pure` option to api docs
  • Loading branch information
nfcampos committed Apr 23, 2016
1 parent 49b221b commit 7a88aea
Show file tree
Hide file tree
Showing 3 changed files with 497 additions and 39 deletions.
1 change: 1 addition & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ const b = a.defaults({ withRef: false })

* `[newOptions = {}]` *(Object)*: An object with any of the following keys:
- `withRef` *(Boolean)*: If `true`, the connector will store a ref to the wrapped component instance and make it available via the `getWrappedInstance()` method. Defaults to `false`.
- `pure` *(Boolean)*: If `true`, the connector will treat the wrapped component and the mapPropsToRequestsToProps function as pure, recomputing requests and re-rendering only when props are shallowly different. If `false`, recompute and re-render every time props change. Defaults to `true`.

### `PromiseState`

Expand Down
13 changes: 12 additions & 1 deletion src/components/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ function connect(mapPropsToRequestsToProps, defaults, options) {
checkTypes(defaults)

options = Object.assign({
withRef: false
withRef: false,
pure: true
}, options)

// Helps track hot reloading.
Expand Down Expand Up @@ -183,13 +184,19 @@ function connect(mapPropsToRequestsToProps, defaults, options) {

componentWillReceiveProps(nextProps, nextContext) {
if (
!options.pure ||
(dependsOnProps && !shallowEqual(omitChildren(this.props), omitChildren(nextProps))) ||
(dependsOnContext && !shallowEqual(this.context, nextContext))
) {
this.refetchDataFromProps(nextProps, nextContext)
}
}

shouldComponentUpdate(nextProps, nextState) {
return !options.pure ||
this.state.data != nextState.data || !shallowEqual(this.props, nextProps)
}

componentWillUnmount() {
this.clearAllRefreshTimeouts()
this._unmounted = true
Expand Down Expand Up @@ -325,18 +332,22 @@ function connect(mapPropsToRequestsToProps, defaults, options) {

return {
startedAts: Object.assign(
{},
prevState.startedAts, {
[prop]: startedAt
}),
mappings: Object.assign(
{},
prevState.mappings, {
[prop]: mapping
}),
data: Object.assign(
{},
prevState.data, {
[prop]: datum
}),
refreshTimeouts: Object.assign(
{},
prevState.refreshTimeouts, {
[prop]: refreshTimeout
})
Expand Down
Loading

2 comments on commit 7a88aea

@ryanbrainard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nfcampos I just got bit by facebook/react#2517 after this commit. I don't think there's much we can do at this point (unless you know of something?), but just something to be aware of and apps will need to use pure: false to workaround it.

@nfcampos
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanbrainard Yeah as you said there's not much we can do here. A couple more workarounds:

  • react-router has implemented Make <Link> work with static containers (take two) remix-run/react-router#3430 for version 3.0 (not yet released) to work around this for the Link component (basically it's something like the parent component that provides context provides a subscribe method so that child components that need the context subscribe to changes in the context and on receiving new context via this event subscription re-render). They have plans to pull this workaround into a separate library soon Extract ContextUtils into react-context-subscriber remix-run/react-router#3484 so if you control the components that use context you can use the same technique.
  • if the component that uses context is the wrapped component (and not a child further down the tree) then we could in our shouldComponentUpdate compare context as well. This would allow people that use context in the wrapped component to use pure: true but doesn't solve anything if you use further down the tree

Please sign in to comment.