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

Refactor dataForChild on DocumentType.Mutation to use new bound funct… #772

Merged
merged 3 commits into from Jul 5, 2017

Conversation

Projects
None yet
4 participants
@baerrach
Copy link
Contributor

baerrach commented Jun 13, 2017

Attempt at fixing #725

dataForChild previously returned a new function for type DocumentType.Mutation
causing React shallowEqual render optimizations to fail.

Refactored the mutation specific code into its own method with a binding to
'this' in the constructor.

Haven't create a test case for this as all the tests render nulls, and this only shows up in real use on multiple renders. I'm not enough of an expert on React and testing to know how to do this properly.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion
  • If this was a change that affects GitHunt-React, update GitHunt-React and post a link to the PR in the discussion
Refactor dataForChild on DocumentType.Mutation to use new bound funct…
…ion.

dataForChild previously returned a new function for type DocumentType.Mutation
causing React shallowEqual render optimizations to fail.

Refactored the mutation specific code into its own method with a binding to
'this' in the constructor.
@helfer

helfer approved these changes Jun 14, 2017

Copy link
Member

helfer left a comment

Thanks @baerrach, I think this is great! I'd be okay with merging this without a test, but if @jbaxleyiii has some ideas for testing, it would be great to add one. I assume testing for re-renders is a solved problem, so there should be some prior art. If not, we should document it and write a blog post about it 😁

@baerrach

This comment has been minimized.

Copy link
Contributor Author

baerrach commented Jun 14, 2017

Yes, I'm all for writing tests with PRs.
And I need to know how to do this for my own work. So I'm keen to learn.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Jun 14, 2017

@baerrach

This comment has been minimized.

Copy link
Contributor Author

baerrach commented Jun 15, 2017

I'm in no rush.

I also just found How to Fork and Patch NPM Modules so I can stop manually editing files in node_modules.

@derek-duncan

This comment has been minimized.

Copy link

derek-duncan commented Jun 21, 2017

@baerrach thanks for tackling this one. I just wanted to note that all of the functions in the data object create a new reference each render. Should we include these in this PR as well?

Functions in data response for queries.

data.refetch(variables)
data.fetchMore(options)
data.subscribeToMore(options)
data.startPolling(interval)
data.stopPolling()
data.updateQuery(updaterFn)
@baerrach

This comment has been minimized.

Copy link
Contributor Author

baerrach commented Jun 22, 2017

Yes, its a can of worms that needs looking at thoroughly.
Its probably not my area of expertise those, so if you can find someone who has the time and knowledge it would be best to get them to look over the code base with that in mind.

First step might be to write tests to make sure things aren't re-rendering unnecessarily, then that will give people a list of things to work on.

With a set of guidelines in place on how to resolve the typical issues other people should be able to chip in and fix it up bit by bit.

James Baxley added some commits Jul 5, 2017

@jbaxleyiii jbaxleyiii merged commit 882959b into apollographql:master Jul 5, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 92.144%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment