Skip to content

Removed shallowEqualScalar, simplified connect update logic #204

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

Closed

Conversation

taylorhakes
Copy link
Contributor

I saw there was a lot of duplicated logic between shallowEqual and shallowEqualScalar. In the process of refactoring, I noticed createConnectDecorator has extra shouldComponentUpdate checks that don't exist in createConnector.

That means

export default class CounterApp {
  render() {
    return (
      <Connector select={select}>
        {({ counter, dispatch }) =>
          /* Yes this is child as a function. */
          <Counter counter={counter}
                   {...bindActionCreators(CounterActions, dispatch)} />
        }
      </Connector>
    );
  }
}

behaves differently than this

@connect(state => ({
  counter: state.counter
}))
export default class CounterApp {
  render() {
    const { counter, dispatch } = this.props;
    // Instead of `bindActionCreators`, you may also pass `dispatch` as a prop
    // to your component and call `dispatch(CounterActions.increment())`
    return (
      <Counter counter={counter}
               {...bindActionCreators(CounterActions, dispatch)} />
    );
  }
}

when props are specified.

I wanted to create this pull request to start a discussion and see if we feel that extra check is necessary. If it isn't, we can remove shallowEqualScalar completely.

@gaearon
Copy link
Contributor

gaearon commented Jul 6, 2015

IIRC it behaves differently because the select function passed to the @connect decorator also accepts the current props of the component. This is unnecessary for <Connector> because you can just read them from this.props.

@taylorhakes
Copy link
Contributor Author

@gaearon The @connect decorator accepts the current props of the component and redux is testing for shallowEqualScalar of the props (similar to adding the PureRenderMixin for them). In the case of <Connector>, the props of the child component aren't accessible to the <Connector> because it is only given a child function, not the actual props.

I would suggest that we don't do the shallowEqualScalar check in the createConnectDocorator. The user of redux can do it themselves (add PureRenderMixin or some other way) in the DecoratedComponent (the component with the @connect decorator attached).

Not only would the change make the @connect decorator and the <Connector> behave the same, it would make the library more flexible. I think that having a shallowEqualScalar check doesn't add much value. There will be very few people that add props to their decorated components (outside of the slice from the stores). If they do add additional props, we should give them full control of how the components updates with those props. We are already doing a shallowEqual check on the slice data for them in the <Connector>.

@taylorhakes taylorhakes force-pushed the shallow-equal branch 2 times, most recently from f252935 to 2345d80 Compare July 9, 2015 05:41
@gaearon
Copy link
Contributor

gaearon commented Jul 12, 2015

We're moving React wrappers to a separate project: https://github.com/gaearon/react-redux.
I'm closing this PR but I'll come back to it in reduxjs/react-redux#2.

@gaearon gaearon closed this Jul 12, 2015
@gaearon
Copy link
Contributor

gaearon commented Aug 7, 2015

similar to adding the PureRenderMixin for them

To clarify, it is not similar. shallowEqualScalar doesn't break people's code if they do deep updates. shallowEqual does. shallowEqualScalar is more “relaxed” but still more performant than doing nothing at all.

We want good perf by default. Relay also uses shallowEqualScalar for its containers.

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

Successfully merging this pull request may close these issues.

2 participants