apollographql / react-apollo

:recycle: React integration for Apollo Client
https://www.apollographql.com/docs/react/
MIT License
6.85k stars 787 forks source link

React Hooks support #2539

Closed vovacodes closed 5 years ago

vovacodes commented 6 years ago

I'd like to start a discussion about the possible next-gen API based on react hooks for react-apollo. I think this API brings a perfect opportunity for react-apollo to improve already excellent DX even more.

// isn't this beatiful?
function Avatar() {
  const { loading, error, data } = useQuery(`
    {
       me {
          initial
       }
    }
  `);

  if (loading) return <div>Loading...<div>;

  if (error) return <div>Error happened</div>

  return (
    <div>{data.me.initial}<div>
  );
} 

What do you guys think?

danielkcz commented 5 years ago

@revskill10 I would say that's kinda blind-folded view on the matter. All three operations ARE different and there are different needs for each of them. Doesn't matter if there is just a fetch underneath (or websocket in case of subscriptions). This is where GraphQL shines and gives you much better DX given some thought.

Apollo has always been focused on better DX. If you want simplicity go with eg. graphql-request from Prisma. There is a bunch of others, just pick your favorite 😉

// or go like this and you don't even need `react-apollo` at all
const client = useApolloClient()
client.query(...)
client.mutate(...)
client.subscribe(...)

I get that things feel difficult for you. At least that's the impression I got from your comment. It's understandable, every one of us has been there. But that's a beauty of our profession, to be evolving and learning. Going with simple is no fun imo :)

What a customer want ? Simplicity, right ?

You know that every customer is different, right? Some people actually enjoy complicated stuff.

So, as a customer, what i expect from the library is the library could do some "smart" things like caching, invalidation, reconnect,... for developers.

Without separating type of operation, library cannot be that smart. Subscriptions usually have a different data shape and need some special treatment. What would "reconnect" mean for queries or mutations?


Just a few bullet points...

Interestingly enough I am currently trying to rewrite a useMutation from the hooks package because it's really not that simple if you want to write some serious app that actually cares about UX.

revskill10 commented 5 years ago

@FredyC If you look again at my API usage

const [data, loading, errors] = useGraphql({uri, headers, query})

This API has some interesting parts:

It's what make ReactJS great since day one: Keep API surface minimal.

Do you care if that query is a query, mutation or subscription ? I guess no. It's the implementation detail that takes care for it. I'm currently use above API for query and subscription, not mutation yet. mutation is an interesting case, and actually, that data for mutation is the action that you will fire later on.

So my point is, having a minimal api surface will prevent big rewrite, keep backward compatibility whatever the underneath change is. It's my point here. If you go fancy way with thing like graphql HOC, it'll sooner or later break its API in some days, the reason is because it's fancy, right ? Let's keep the core minimal, and let's community enhance it with community packages.

danielkcz commented 5 years ago

Alright, have fun with graphql-request then. Make yourself a tiny custom hook wrapper to handle loading/error states and you are good to go. 🐌 Apollo is overkill for your needs, but I am convinced you will eventually realize the mistake of your decision.

MrLoh commented 5 years ago

@revskill10 actually having a super minimal API surface as you suggest it will lead to everything becoming magic behind the scenes and nothing being explicitly agreed. As @FredyC said before these operations are vastly different and have vastly different needs, there are no fetch policies for mutations or subscriptions, errors mean something different etc.

I think you have a fundamental misconception about what Apollo does. Apollo’s purpose is not to send GraphQL requests, that is super easy to do with fetch yourself, you could write a basic hook as you describe it in 20 lines of code. Apollo’s purpose is first and foremost to create a global cache and subscribe components to that state (not talking about subscriptions here, but queries subscribe to the cache so you just update the cache wherever you want and your components will rerender, queries in Apollo are not like network requests but like redux selectors) and then Apollo also does error handling and customizing the network layer like refetching.

So what are you Talking about? The client is the very core of what Apollo is, you cannot simply hide it. How could one merge queries and mutations, the ones are like selectors in redux the other like action dispatchers and you define the accompanying reducer with the update function, so queries and mutations have nothing in common. Maybe Apollo needs to do a better job of explaining the whole state management part that it is about, so people don’t mistake it for a GraphQL request library.

revskill10 commented 5 years ago

Thanks @FreddieRidell for suggestion. I'll just keep fetch and cache the hashed query in Redux to rehydrate on client later. @MrLoh I never say we don't need a client, i just say it should be hidden from api surface. Just in fetch, we just need to fetch(url, options) and no need to care what happens behind the scene. In my component, i need to use multiple graphql endpoints, so this one requires overhead to keep track of multi clients, and i think it should be the job of a library. (What if i want to reuse those clients in other components ?)

danielkcz commented 5 years ago

In my component, i need to use multiple graphql endpoints, so this one requires overhead to keep track of them, and i think it should be the job of a library.

Have you ever heard about graphql stitching? :) That's generally what solves the "problem" of multiple endpoints, just join those together. It is indeed harder to solve this client side, so that's why there are tools to tackle it elsewhere more gracefully.

Nonetheless, this is getting off topic. You are happy with your choice and we are happy what Apollo does for us. It's a win-win ;)

mbrowne commented 5 years ago

It is indeed harder to solve this client side, so that's why there are tools to tackle it elsewhere more gracefully.

This is true, but actually handling this client side (if that's a requirement) is very doable with the features already provided by apollo-client and apollo-link. I would recommend that anyone interested in discussing that further check out the Spectrum community and maybe start a new thread there.

oklas commented 5 years ago

@FredyC, if we will integrate as suggested two object or list queries into one request, it is impossible to refetch data for only one query in that request.

@rovansteen:

So I think it makes most sense to stick with the current objects until Suspense simplifies everything massively.

Yes we are currently discussing future api, hooks seems not yet merged. So array api may be like this:

  const [ dogs, dogsRefetch, { fetchMore: dogsMore } ] = useQuery(GET_DOGS)

It is like just functions. It may be actually considered as function params. It is just params of callback function but converted to return value. Too much functions have some position parameters and object parameters. for example, not need to go so far, fetch itself:

  fetch('/endpoint', { method: 'GET', headers })

The question is: is it possible to extract some small amount of API as frequently used to position parapeters in tuple?

oklas commented 5 years ago

The way to use namespacing by prefix in object form:

const { lecturer, lecturerRefetch } = useQuery(GET_LECTURER, {prefix: 'lecturer'})
const { studentList, studentListRefetch } = useQuery(GET_SDUDENTLIST, {prefix: 'studentList'})
danielkcz commented 5 years ago

@oklas So because of some theoretical need to run multiple queries in a single component you would have botched the API like that? 😮 Seriously, how many components with multiple useQuery have you seen so far?

We are in the middle of refactoring some medium sized app and there was not a single occasion to need that. If more data is needed, we rather make a single merged query which is way easier to work with (single loading, error...).

Sure, there might be some cases where one query depends on another, but I am in favor of separating that into another nested component that receives data from the previous one through props. SRP is a thing :)

oklas commented 5 years ago

Any component which manage relations between more then one different objects

danielkcz commented 5 years ago

The way to use namespacing by prefix in object form:

const { lecturer, lecturerRefetch } = useQuery(GET_LECTURER, {prefix: 'lecturer'})
const { studentList, studentListRefetch } = useQuery(GET_SDUDENTLIST, {prefix: 'studentList'})

This would be a nightmare for the TypeScript/Flow world... 😱

Yea, as I said theoretical until proven otherwise and even then it's upon heavy consideration if that couple of rare cases is worth redesigning API and making it harder for others.

MrLoh commented 5 years ago

A quite common and very valid use-case for several queries in one component ist query splitting (see docs here).

We have that in our app in a few places. A list view renders items with a little data, then if an item is clicked, the detail view uses a query that gets the data that’s already in cash to display it and a second query to load more data over the network. This is super ugly with HOCs as you can see in the docs.

I actually like the list return from hooks as it reflects how useState works, but as already mentioned it will be easy to customize with your own hook. Probably we will refactor all data fetching code out of our components into shared custom hooks that will get data and can be reused to more easily optimize things like query splitting.

danielkcz commented 5 years ago

Interesting, I always thought that Apollo is slightly smarter in this and it actually requests only data that are not in cache (unless fetchPolicy dictates otherwise). I was never curious enough to investigate it 😅 Ok, that is certainly a valid case, but still, there isn't probably many of those.

Either way, the point is that the current API is not preventing anyone from running multiple queries. It's just slightly less convenient/verbose to do it. And if someone does not like it, there is indeed a super easy way of abstracting it with a custom hook.

function useTupleQuery(...) {
  const { data, loading, error, ...rest } = useQuery(...)
  return [ data, loading, error, rest ] 
}

It's so tiny that there is no problem to have it copy-pasted in userland code. The beauty is that you can easily choose which variant to use based on the scenario in the component. You can even mix both. Sure, even if API would be other way around, we could do the opposite. The main reason why I don't like that is that the tuple variant will be always more opinionated due to an order of arguments which can be a source of weird errors.

CrocoDillon commented 5 years ago

The main reason why I don't like that is that the tuple variant will be always more opinionated due to an order of arguments which can be a source of weird errors.

This ^ Imagine if the Query render prop would use tuples

<Query {...props}>
  {([i, dont, know, the, order]) => <Thing />}
</Query>

That looks so weird, but now with hooks it's suddenly different. Maybe the best thing is just to keep the API similar to the Query render prop so you don't even need to think about destructure order.

DanielFGray commented 5 years ago

As far as bikeshedding the API of the tuple goes, I think to align with the native hooks,

const [{ data, loading, error, ...rest }, refetch] = useQuery(...)

makes the most sense, considering most are in the form of [state, changeState].

revskill10 commented 5 years ago

For anyone interested, here's the gist i'm using: https://gist.github.com/revskill10/d313f30771b8eeb2bc423867c300fe9c with the API i suggest above. All graphql operation has this form:

const [action, data, loading, error] = useFetchQL(
  {key, query, mutation, subscription,...}, callback
)

Surprisingly, there's no difference at all between query, mutation, or subscription key is the key in the cache map. Callback is called after operation completed. action is the action, for query it's the refetch, for mutation, it's the mutation, for subscription, it's the subscribe It's Suspense ready, i tested with SSR suspense, too. Demo is here , with SSR, caching, realtime in one component. For subscription, i'm temporarily using apolloClient though, but disable its cache. Will replace with native websocket implementation later. Query and Mutation just use native fetch.

Here's another use of useFetchQL,

const [data] = useFetchQL({
    key: 'top-10-posts',
    fetch: async () => {
      const url = 'https://jsonplaceholder.typicode.com/posts?_page=1&_limit=10'
      return await fetch(url)
    }
  });

It supports generic async call, too.

So, let's just see that, don't treat graphql at any difference with another. From the consumer point of view, Graphql is just a string inside your body string, treat it no difference to another , old school fetch that we used and loved in the past.

oklas commented 5 years ago

Easy expansion of the existing API with format-like method(s):

const [data, refetch] = useQuery(...).asState()
danielkcz commented 5 years ago

@oklas I don't think it's even worth considering to include that in the core. Same reason why React does not include hundreds of utility hooks. Everyone has different needs and opinions on how API should look like. It's great we have custom hooks and you can shape it however you like without bothering others with it. Let's stop this discussion as it doesn't really lead to any useful end.

fbartho commented 5 years ago

@fredyc if what you’re saying is: react-Apollo should have an official hooks implementation, but that we should stop quibbling as to the exact syntax, because anybody can wrap that to their liking. Then I agree!

Nearly any official react-apollo hooks strategy is enough here!

jigsawye commented 5 years ago

Maybe the return value can try this:

function useQuery() {

  // ...

  const ret = [data, loading, error]; // or something else
  ret.data = data;
  ret.loading = loading;
  ret.error = error;
  // or something else

  return ret;
}

const [data, loading, error] = useQuery(); // work
const { data, loading, error } = useQuery(); // also work
sessionboy commented 5 years ago

This is very good:

const [{ data, loading, error, ...rest }, refetch] = useQuery(...);

If the array has more than two elements, it's going to be a little tricky. For example, I just want one and four:

// Get all :
const [data, loading, error,refetch] = useQuery();

// Get one and four. This is bad
const [data, , ,refetch] = useQuery();
slikts commented 5 years ago

Valid syntax for eliding members when destructuring is:

const [data, , , refetch] = useQuery();
danielkcz commented 5 years ago

Please, let's leave these ideas of tuples. The useState has kinda opened hell gate and now everything needs to a be a tuple while before there weren't many people who wouldn't think about such an approach. It certainly has its appeal for some use cases, but seriously, for different arguments is a total nightmare. Who is going to decide what's the "correct" order? Are you always going to remember what's the order?

I don't really follow why is so hard for you to just do this in your app/lib code. It's an absolute win-win for everyone.

function useTupleQuery(...) {
  const { data, loading, error, refetch, ...rest } = useQuery(...)
  return [ data, loading, error, refetch, rest ] 
}
ryandrewjohnson commented 5 years ago

@hwillson Thoughts on locking this thread and moving these other discussions over to apollo's official Spectrum channels? I'm subscribed to this thread to receive updates on the official hooks implementation, and these other convos are blowing up my notifications. Sure I can unsubscribe, but than I lose visibility on the updates I'm really here to receive like:

https://github.com/apollographql/react-apollo/issues/2539#issuecomment-464025309

bmullan91 commented 5 years ago

First off kudos to the Apollo team with the updates and clear roadmap. I've used Apollo on a few projects and it's worked great.

We've been working on a project graphql-hooks. It's a super lightweight GraphQL client with first class hooks support.

We'd love to get some feedback, and if anyone would like to get involved please get in touch!

PS: great job on react-apollo-hooks @trojanowski

maxlew commented 5 years ago

We use multiple GraphQL queries in a single component quite a lot in our large app. However I agree with @FredyC - ours is a super niche situation and considering our projects complexity it's not too much to expect us to create our own useTupleQuery wrapper. Having any kind of spec is better than arguing about API implementations.

revskill10 commented 5 years ago

I sketch out basic implementation here Basically, it has the following API:

const [{
  json, error, loading
}, {
  refetch, abort, setQuery, setTimeout, setVariables, setInit, setOperationName
}] = useGraphql({
  key, url, query, variables, operationName, init, skip, timeout
}, {onComplete, onError, onAborted})

The benefit is, we can easily change everything we want, from query, timeout, variables, headers, operation name in one component. With this, a custom GraphiQL is easily done !

danielkcz commented 5 years ago

@revskill10 Seriously man, no offense, but nobody cares about your "I use graphql like a fetch", so please stop spamming with it and spreading it like some holy grail. If you really like it, make your own lib and perhaps you will be famous, but it's not funny anymore.

MrLoh commented 5 years ago

@hwillson please lock the thread!

hwillson commented 5 years ago

There has been a lot of great discussion here, but this thread is definitely getting a bit unwieldy. We'll lock it for now, and post back when we have updates to share. Let's keep the discussion going over in https://spectrum.chat/apollo. Thanks!

hwillson commented 5 years ago

Hooks support is almost ready. Anyone interested can follow along in https://github.com/apollographql/react-apollo/pull/2892. We'll be merging into master very shortly, and releasing as soon as we get the docs updated. Thanks all!