-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
withApollo example - move from old HOC APIs to new function-as-child APIs #5241
withApollo example - move from old HOC APIs to new function-as-child APIs #5241
Conversation
A little more on why I used the |
@amytych @joaogarin I see from #5238 that you're working with this example now.. care to give any feedback? |
@adamsoffer could you have a look 🙌 |
If this PR is approved I can follow up with the same updates to the other Apollo examples (with-apollo-auth, with-apollo-and-redux, with-apollo-and-redux-saga) |
if (error) return <ErrorMessage message='Error loading posts.' /> | ||
if (loading) return <div>Loading</div> | ||
|
||
function loadMorePosts () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this function declaration outside of and below PostList
? Everything else looks awesome to me. Thank you!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsoffer Sure thing! I'm guessing you'd like the same done in the modules PostUpvoter (upvotePost function) and Submit (handleSubmit function)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes those as well thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsoffer done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👊
@timneutkens are we waiting for more reviews before merging? |
Looks great 👍 Thanks for the PR 👌 |
Awesome! I'll follow up with the same changes to the other Apollo examples in a single PR. |
I honestly don't like this approach at all. You can for the most part avoid the complexity problem by breaking your components artificially into smaller ones, where otherwise it wouldn't even make much sense to do that, but then you end up with a ton of components, which brings the question of whether that is making things more manageable or not. And then there're some HOCs you might still want to use (there're some really cool ones out there), even if you do adopt this pattern, so you'll have two different approaches living side by side. So, as interesting as this idea might seem in theory, I personally would not recommend it to anyone, especially not on medium to large sized projects. Just my two cents. |
@thealjey I hear that. I imagine the apollo team is working hard on hooks support and I'm looking forward to updating this example when that's ready. |
Since version 2.1, react-apollo is exposing some new components that use the function-as-child (or render-prop) pattern to let you connect apollo-client magic with your components. See the blog article: New in React Apollo 2.1
If I'm not mistaken, it's generally agreed that this pattern is (where it works) superior to the HOC pattern, for reasons that are best explained here: https://cdb.reacttraining.com/use-a-render-prop-50de598f11ce
So I updated the with-apollo example to use the new API, and IMO this code is much simpler and natural to read and understand, especially if you are not already familiar with Apollo's HOC APIs.
I broke up my changes into separate commits, for easier review. Commits with "Refactor" in the message accomplish the goal of switching to the new APIs while minimizing line-by-line differences (select "Hide whitespace changes" under "Diff settings"). Commits with "Clean up" in the message follow up the refactoring with trivial things like reorganizing code sections, renaming variables, etc.
For the components doing mutations, I chose not to use the
Mutation
component, since that doesn't really make sense to me; a mutation is something that happens at a point in time, so it's not meaningful to represent a mutation in the markup, which exists for a period of time. All that component does is expose amutate
function for a single specified mutation, andresult
data for a single firing of the mutation (which we don't need anyways; apollo handles updating the local data with the result). To me it seems simpler and more flexible to just get the apollo client viaApolloConsumer
and call.mutate()
on it.In case anyone is interested, here's what my version of
PostUpvoter
using theMutation
component looked like:I'm happy with where things are at here, but I'm more than happy to address any comments, concerns, ideas for improvent!
Thanks!