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

Remove superfluous condition in GraphQL.shouldComponentUpdate() #653

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jamesreggio
Copy link
Contributor

jamesreggio commented Apr 24, 2017

While profiling a React Native app, I discovered a significant number of re-renders occurring for components decorated by the graphql HOC.

The proposed change is small, but with great potential for regression. Here is my reasoning for why this is safe — please scrutinize carefully:

  • The !!nextContext condition will always evaluate to true because non-null contextTypes are required by the HOC. As such nextContext will always be an object containing a {client}.

  • The componentWillReceiveProps method 'owns' the setting of this.shouldRerender and already includes a check for updates to the {client} in the nextContext.

I cannot intuit why !!nextContext was ever in this function, but it probably relates to code that is long dead. (Perhaps it traces back to a time before componentWillReceiveProps checked for a change to the client?)

If this change seems peaceful, I think it could be a boon to performance.

I'm not certain of a good way to test this, but am open to suggestions.

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Apr 24, 2017

@jamesreggio: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Apr 29, 2017

@jamesreggio I remember why we added this now. Its needed to update when other context change. See https://github.com/apollographql/react-apollo/blob/master/test/react-web/client/graphql/queries.test.tsx#L1567-L1637 as a test that fails when this is removed

@jbaxleyiii jbaxleyiii referenced this pull request Apr 29, 2017

Merged

shouldComponentUpdate perf #665

5 of 5 tasks complete
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Apr 29, 2017

closed for #665

thanks @jamesreggio!!

@jbaxleyiii jbaxleyiii closed this Apr 29, 2017

@jamesreggio jamesreggio deleted the jamesreggio:fix-graphql-perf branch Apr 29, 2017

@jamesreggio

This comment has been minimized.

Copy link
Contributor Author

jamesreggio commented Apr 29, 2017

Thanks for shepherding this through!

@jbaxleyiii jbaxleyiii referenced this pull request May 1, 2017

Merged

Release 1.1.3 #676

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