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

v2.1 Implemented the Query Component #1398

Merged
merged 16 commits into from Dec 22, 2017

Conversation

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

excitement-engineer commented Dec 10, 2017

I was checking out the Roadmap for v2.1 of React-Apollo and I am really excited about all the features coming in the next version, so I decided to implement some of the ideas described:)

The Roadmap describes the Query component that will allow use of this library without resorting to HOC. This PR provides an implementation of this Query component in line with the API described in the roadmap.

I have implemented most of the features that I think the Query component will need. I would love to hear some feedback on what I missed and on how to improve the API!

As an example, you can now interact with apollo like this:

const Component = () => (
      <Query
        query={query}
        loading={() => <div>Loading</div>}
        error={error => <div>{error}</div>}
        render={result => {
          return <div>Data:)</div>;
        }}
      />

I have implemented a fair amount of tests for the component but it is not complete yet, this PR is still very much a WIP. In addition, I have not tried it yet in an actual implementation yet, however I will implement an new repo in the examples folder that will make use of this API to test it out .

I would love to hear what you think!

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 10, 2017

Fails
🚫

No CHANGELOG added.

Warnings
⚠️

❗️ Big PR

⚠️

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

Generated by 🚫 dangerJS

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 15, 2017

I definitely like the render callback as a child like i18next uses, it makes the code cleaner with an end tag.

const Component = () => (
  <Query 
    query={query} 
    loading={() => <div>Loading</div>} 
    error={error => <div>{error}</div>}
  >
    {result => {
      return <div>Data:)</div>
    }}
  </Query>
)
OptionProps,
} from './types';

type Props = {

This comment has been minimized.

@rosskevin

rosskevin Dec 15, 2017

Collaborator

Use an interface and it will need to be exported when tsc -d runs and creates the .d.ts file. I usually name these after the component so they are more useful in autocomplete, such as QueryProps here - aids is composition of any components that might use these.

};
_updateCurrentData = () => {
this.setState({ result: this.queryObservable.currentResult() });
};

This comment has been minimized.

@rosskevin

rosskevin Dec 15, 2017

Collaborator

For these methods, instead of underscore, use private updateCurrentData to scope them. I may PR a tslint update that will auto fix the current codebase.

return error(result.error);
}

return render(result);

This comment has been minimized.

@rosskevin

rosskevin Dec 15, 2017

Collaborator

I much prefer the render callback to execute children, as noted in general comments.

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 16, 2017

Author Collaborator

Refer to my comment in #1399 about my thoughts here. I will leave it as a render prop for now and await feedback from some more people before updating anything

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 15, 2017

Thanks for taking time to work on this, definitely something I would like to see.

@excitement-engineer excitement-engineer changed the title Implemented the Query Component v2.1 Implemented the Query Component Dec 16, 2017

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 16, 2017

Hey @rosskevin, thanks a lot for your feedback, this helped improve the code! I really appreciate it:)

I have created an example project illustrating the usage of the new component under /examples/components.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 18, 2017

@excitement-engineer now that we are both committers, we should probably hash out with @jbaxleyiii the style choice on the render callback. I'm firmly in the camp of using children.

My argument is simple:
React components render children - so adding another prop doesn't add value. If it did add value, then the react component API would have used render as a prop instead of children.

Counter arguments?

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 20, 2017

Warnings
⚠️

❗️ Big PR

⚠️

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 20, 2017

Hey @rosskevin @jbaxleyiii ! I have added support for both a render-prop and children render function. A lot of libraries support both conventions and now people can choose the one they prefer. What do you think?

I think we also need to take a look at the loading, and error render callback. I think that they add unnecessary complexity to the API whilst their benefit is not immediately clear to me.

Consider:

<Query
    query={HERO_QUERY}
    loading={() => <div>Loading</div>}
    error={() => <h1>ERROR</h1>}
    render={result => <div>data</div>}
/>

vs

<Query
    query={HERO_QUERY}
    render={result => {
      const { loading, error, data } = result;
      if (loading) {
        return <div>Loading</div>;
      }
      if (error) {
        return <h1>ERROR</h1>;
      }

      return <div>data</div>;
/>

I think that it is important to keep the API as simple as possible for users. Therefore, I am leaning towards the second API (without the loading and error callback). Any thoughts?

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 20, 2017

Also, should we go ahead and merge this PR and continue on refining in smaller follow-up PRs?

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Dec 20, 2017

@excitement-engineer Given the fact that loading and having data are not exclusive (https://www.apollographql.com/docs/react/basics/queries.html#graphql-query-data-loading) (because you could already have data, and be refetching to get newer data, thus loading and still having some data to show), I think it's more flexible to use a single prop (be it render or children) and pass the entire result object, and let the function handle the different cases. That way you could have a loading without data state, and a loading with data state, if you want.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 21, 2017

Good point @leoasis, I think that a single prop render callback is better in this case then! Thanks for your comment

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 21, 2017

I am also wondering whether can simplify the API further. Instead of using an options prop. Perhaps it is an idea to move all the individual keys of the options object as props. So instead of

  <Query
    query={HERO_QUERY}
    options={{
      variables: {
        episode,
      },
    }}
    render={result => null}
/>

We could do:

<Query
    query={HERO_QUERY}
    variables={{episode}}
    render={result => null}
/>

Thoughts?

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Dec 21, 2017

@excitement-engineer I think that's a good idea, since there are not that many options anyway, and now that we have the props as the component API we can use that. Also some options, such as skip would no longer be useful in this context (since you can always just not render a component)

@rosskevin
Copy link
Collaborator

rosskevin left a comment

Thanks again for your work on this. I added a few notes - I feel strongly we should pick an implementation (prop) to prevent a larger API surface area and confusion from users. As I mentioned previously, children is the the prop everyone already knows that React renders, so while some others have chosen the render prop explicitly, I think it is redundant and the wrong path. If someone really wants that in their code, they can wrap our component and convey their render prop to our children prop. Also, being really picky about type names now will save us loads of issues and user confusion later as well.

skip?: Boolean;
loading?: () => React.ReactNode;
error?: (error: ApolloError) => React.ReactNode;
render?: ((result: QueryRenderProp) => React.ReactNode);

This comment has been minimized.

@rosskevin

rosskevin Dec 21, 2017

Collaborator

I'm in favor of omitting this

loading?: () => React.ReactNode;
error?: (error: ApolloError) => React.ReactNode;
render?: ((result: QueryRenderProp) => React.ReactNode);
children?: ((result: QueryRenderProp) => React.ReactNode) | React.ReactNode;

This comment has been minimized.

@rosskevin

rosskevin Dec 21, 2017

Collaborator

This should be function only, no | React.ReactNode - it only confuses the possibilities.


export interface QueryProps {
query: DocumentNode;
options?: QueryOpts;

This comment has been minimized.

@rosskevin

rosskevin Dec 21, 2017

Collaborator

I definitely agree that we should flatten the options here - no need for a nested options object.

import { QueryOpts, OperationVariables } from './types';
import { parser, DocumentType } from './parser';

export interface QueryRenderProp {

This comment has been minimized.

@rosskevin

rosskevin Dec 21, 2017

Collaborator

I think we should rename this type to prevent confusion. I suggest:

  • QueryResult (preferred)
  • QueryRenderArgs
@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Hey @rosskevin. Thanks a lot for your review! I will update the PR accordingly. You have convinced me about using children as the render callback, I have removed the render prop from the component. I completely agree that we should keep the API as simple as possible, thanks for guarding simplicity!

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

With regards to simplicity of the API, you put up a good point @leoasis, I think you are right about being able omit the skip prop completely when it comes to render props, since you can conditionally render the query component.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

I am also wondering if there is a use case for including an HOC version of the Query component, you can do everything with a render prop that you can do with an HOC so why should it be included in the library? An HOC will only increase the API surface area of the library and confuse users who now need to choose between one or the other. Thoughts?


return null;
const renderedChildren = children(result);
return renderedChildren && React.Children.only(renderedChildren);

This comment has been minimized.

@leoasis

leoasis Dec 22, 2017

Collaborator

Do we still have to have this limitation with React 16? We could allow more than one resulting child since now React allows it

This comment has been minimized.

@rosskevin

rosskevin Dec 22, 2017

Collaborator

I'm fine with limiting to 16+ - but I'm on the progressive side. Not sure how other users would feel about it.

This comment has been minimized.

@leoasis

leoasis Dec 22, 2017

Collaborator

I'd say yes, since this will be part of a new version, a new API. And even then, you can still use it in previous versions, only that it will fail with a less clear error when you attempt to render multiple children than the one React.Children.only gives you

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

@excitement-engineer I had a long discussion about this in material-ui - I don't want to repeat so I'll link: mui-org/material-ui#9503 (comment)

TL;DR - yes, we also want a HOC that wraps the render callback. There are few cases where it is a better user experience, but still useful in those cases.

@rosskevin
Copy link
Collaborator

rosskevin left a comment

Great work, let's get these final things sewn up (like build errors) and I'm going to try it right away in our app. We are actively migrating from react-relay and would rather use render callbacks!

fetchPolicy?: FetchPolicy;
pollInterval?: number;
notifyOnNetworkStatusChange?: boolean;
children?: (result: QueryResult) => React.ReactNode;

This comment has been minimized.

@rosskevin

rosskevin Dec 22, 2017

Collaborator

Children is required, so drop the ? here.


render() {
const { children } = this.props;
const result = this.getRenderProps();

This comment has been minimized.

@rosskevin

rosskevin Dec 22, 2017

Collaborator

I would just put getRenderProps implementation into render. Alternatively you could keep it and rename it to getResult to be consistent.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

@excitement-engineer we can do the HOC in a different PR - I'm anxious to merge and start using to get this thing bulletproofed.

excitement-engineer and others added some commits Dec 22, 2017

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

@excitement-engineer I updated your branch with master.

Pull the update then run yarn pretty && yarn lint-staged to fix the remaining issues to get a green build.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Hey @rosskevin and @leoasis. I incorporated your feedback, thanks! I will get the build to pass and then we can merge this thing for testing purposes.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Regarding HOCs. In the mui comment you said:

BUT, with that said, there are cases (probably 10%) where a HOC is a nicer pattern of use (albeit not a better pattern to have to maintain). Since any render callback pattern can also be easily wrapped and exposed as a HOC, it seems like a no-brainer to do.

I'm curious what the concrete use cases are where HOC is better than a render prop. I think that we need to be careful about including an HOC in the library especially for newcomers to the React world to whom the choice will only cause confusion. We can discuss in a follow-up PR thought:)

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

I'm curious what the concrete use cases are where HOC is better than a render prop. I think that we need to be careful about including an HOC in the library especially for newcomers to the React world to whom the choice will only cause confusion. We can discuss in a follow-up PR thought:)

The use case (for this) would be where the child is a React.Component with event handlers triggered from it's children, and the data is needed to be processed in those handlers -- essentially the provided data is used outside of render(). Sometimes you can convey that data from the render method, other times it is not possible (workaround is to use this.data = data inside render).

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

@excitement-engineer I wonder if you are fighting with lint-staged for some reason. Your last commit had no changes, so perhaps we have a problem in the pre-commit hooks.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Yeah I am having issues with linting. Prettier is automatically adding semicolons to the private functions in the react component which in turn causes the linting to given an error than an unnecessary semicolon has been added.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

I am checking if there is some linting rule that we can turn off in the tslint.json, perhaps you have an idea?

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

I'd like to make changes to our lint. In these cases, I think we should trust one and make the other lenient. I propose semicolons are the responsibility of prettier (and easier that way to make bulk changes), so tslint should become agnostic on the subject.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Completely agree, I think that we should let prettier do what it is good at: formatting, and let typescript do the rest.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

I have pushed another commit, let's see if this one passes.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Just got to add a changelog and we are good to go

@excitement-engineer excitement-engineer merged commit 9d143c9 into apollographql:master Dec 22, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./dist/index.min.js: 6.94KB (467B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@excitement-engineer excitement-engineer deleted the excitement-engineer:Query branch Dec 22, 2017

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 22, 2017

@excitement-engineer your Query.js unit test wasn't running because it wasn't named Query.test.js and there are several failures. If you could PR that fixed, I will work on a typescript conversion and getting it working with parameterized types.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 22, 2017

Oh I didn't see that, thanks! I will do so as soon as I find some time.

@AndrewHenderson

This comment has been minimized.

Copy link

AndrewHenderson commented Feb 9, 2018

@excitement-engineer @rosskevin Is there a new approach to errorPolicy when using the Query component?

I'm trying to use the new component, but my existing query needs to set that option to all . https://www.apollographql.com/docs/react/features/error-handling.html#policies

Using the all policy is the best way to notify your users of potential issues while still showing as much data as possible from your server. It saves both data and errors into the Apollo Cache so your UI can use them.

@AndrewHenderson

This comment has been minimized.

Copy link

AndrewHenderson commented Feb 12, 2018

@excitement-engineer @rosskevin Does the Query component need the errorPolicy prop added? If so, I can work on a pull request. New to TypeScript, but hey! 😄

If I'm simply not understanding the new approach to errorPolicy, let me know. Thanks!

@excitement-engineer excitement-engineer referenced this pull request Feb 20, 2018

Closed

[Query]: Implement skip prop #1694

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment