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

Implemented the ApolloConsumer component. #1399

Merged
merged 6 commits into from Dec 26, 2017

Conversation

Projects
None yet
3 participants
@excitement-engineer
Copy link
Collaborator

excitement-engineer commented Dec 10, 2017

The roadmap for v2.1 describes moving to a component based API (Query, Mutation and Subscription) in addition to the HOCs.

I implemented the component associated with the existing HOC withApollo called ApolloConsumer. This will allow users access to the apollo-client instance using a render prop as follows:

<ApolloConsumer render={client => {
    return <div>data</div>
}} />

I got my inspiration from react-router's API. They have a Route component and a withRouter HOC. The implementation of the HOC is simply a wrapper around the Route component. The same could be done for withApollo, it can simply wrap the ApolloConsumer.

Curious to hear how people feel about such a component and the API proposed above.

Note, this is a WIP. Tests still need to be added.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 11, 2017

Style-wise you could use children as the render call back, no need to add a render prop. Adding a render prop is more verbose but achieves the same thing. react-i18next is a good example of this.

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 15, 2017

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 15, 2017

Hey @rosskevin, this is indeed a style issue. I know that react router uses the "render" prop pattern for example. I will leave it the way it is for now and wait for feedback from others:)

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 24, 2017

I updated the component to use children for rendering in favor of a render prop.

@rosskevin
Copy link
Collaborator

rosskevin left a comment

Looks good, just one thing on the return type.

const invariant = require('invariant');

export interface ApolloConsumerProps {
children: (client: ApolloClient<any>) => React.ReactElement<any>;

This comment has been minimized.

@rosskevin

rosskevin Dec 24, 2017

Collaborator

I think the return should React.ReactNode - it is looser than ReactElement and we have no real concerns about the return type.

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Author Collaborator

Agreed, as soon as I update the type to React.ReactNode I get the typescript error:

Type '(props: ApolloConsumerProps & { children?: ReactNode; }, context: any) => ReactNode' is not assignable to type 'StatelessComponent<ApolloConsumerProps>'.
  Type 'ReactNode' is not assignable to type 'ReactElement<any>'.
    Type 'string' is not assignable to type 'ReactElement<any>'.

It seems that StatelessComponent not accept a ReactNode as the return type. Any ideas on how to fix this?

This comment has been minimized.

@rosskevin

rosskevin Dec 26, 2017

Collaborator

That's mistake then, it should match render's return type. I was thinking it would be node because 16 allows arrays.

This comment has been minimized.

@rosskevin

rosskevin Dec 26, 2017

Collaborator

Should say my mistake - I'm mobile

invalid

@rosskevin rosskevin merged commit 1ae46f1 into apollographql:master Dec 26, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/react-apollo.browser.umd.js: 6.4KB (59B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment