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

React Hooks support #2539

Open
wzrdzl opened this Issue Oct 26, 2018 · 89 comments

Comments

Projects
None yet
@wzrdzl
Copy link
Contributor

wzrdzl commented Oct 26, 2018

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?

@wzrdzl

This comment has been minimized.

Copy link
Contributor Author

wzrdzl commented Oct 26, 2018

Looks like the first step to make it happen is to start using the react 16.3 context API, I opened #2540 to address this

@kristianmandrup

This comment has been minimized.

Copy link

kristianmandrup commented Oct 27, 2018

Yes, the Mutation and Query tags should also be available as useMutation and useQuery :)
Let's do it!

@kristianmandrup

This comment has been minimized.

Copy link

kristianmandrup commented Oct 27, 2018

I started a very basic attempt at hooks here: https://github.com/kristianmandrup/react-apollo/tree/hooks

Feel free to continue this effort. So far extracted Query and Mutation as classes so they are not React Components any longer but can be used as standalone classes.

Also added dependencies to 16.7 alpha versions of react and react-dom

    "react": "^16.7.0-alpha.0",
    "react-dom": "^16.7.0-alpha.0",
@lifeiscontent

This comment has been minimized.

Copy link

lifeiscontent commented Oct 27, 2018

I also opened up this issue here. should I close it? apollographql/apollo-feature-requests#64

@wzrdzl

This comment has been minimized.

Copy link
Contributor Author

wzrdzl commented Oct 27, 2018

@lifeiscontent no idea, would love to hear from the maintainers. it makes sense to close one of the issues to keep the discussion in one place, but I don't really know which place is the right one

@kristianmandrup

This comment has been minimized.

Copy link

kristianmandrup commented Oct 27, 2018

The main effort lies in updating the tests to test the hooks. The implementation itself is mostly just wrapping existing functionality with a few changes. I did much of the basic work, but tests remain to be refactored which I've started.

See my hooks branch if you want to give it a go.

@kristianmandrup

This comment has been minimized.

Copy link

kristianmandrup commented Oct 27, 2018

I now have ~70% of the grunt work done here. Should be able to get it fully working and all tests passing, with ~3-4 hrs more work... Would love to see someone fork it and continue. Over and out here: midnight in London.

@kristianmandrup

This comment has been minimized.

Copy link

kristianmandrup commented Oct 28, 2018

Added usage docs and examples for current implementation attempt: Apollo Hooks

@aralroca

This comment has been minimized.

Copy link

aralroca commented Oct 28, 2018

I like a lot the proposal 😍

@hyperparsa

This comment has been minimized.

Copy link

hyperparsa commented Oct 29, 2018

Integration with Suspense will also be nice.

@albertorestifo

This comment has been minimized.

Copy link

albertorestifo commented Oct 29, 2018

@hyperparsa Suspense is not yet ready to be used with data fetching.

@trojanowski

This comment has been minimized.

Copy link
Contributor

trojanowski commented Oct 29, 2018

I created sample hooks to use with Apollo Client: https://github.com/trojanowski/react-apollo-hooks.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Oct 30, 2018

Even though Suspense is not ready for data fetching, isn't it like double work to do it with hooks first and then rewrite to Suspense? I mean if you look at this video from React Conf (time included) it's kinda clear that any data fetching won't be just a simple hook. Andrew even mentions Apollo there and that it should probably wait for a proper implementation.

@wzrdzl

This comment has been minimized.

Copy link
Contributor Author

wzrdzl commented Oct 30, 2018

@FredyC that's a very good point, that's why it would really be great if the maintainers would share their vision before we go nuts with our implementations and PRs :) I understand the excitement everybody feels about Hooks but we need to plan better what should the ideal API look like @jbaxleyiii

@prateekrastogi

This comment has been minimized.

Copy link

prateekrastogi commented Oct 31, 2018

getting crazy for hooks. Can we resolve this issue on priority basis?

@FredyC

This comment has been minimized.

Copy link

FredyC commented Oct 31, 2018

@prateekrastogi If you are so crazy, just use a package from @trojanowski but be warned it will most likely become obsolete and completely different.

https://github.com/trojanowski/react-apollo-hooks.

@prateekrastogi

This comment has been minimized.

Copy link

prateekrastogi commented Oct 31, 2018

@FredyC I did checked that package, and that was my only reservation about it. :-)

@FredyC FredyC referenced this issue Oct 31, 2018

Closed

Hooks support? #105

@maxguzenski

This comment has been minimized.

Copy link

maxguzenski commented Oct 31, 2018

I've made a very basic useQuery/useMutation that is working on my medium/large project (not in production yet)

https://gist.github.com/maxguzenski/f23f7bba4c726ea5956b17fd3917fa1c

@juank11memphis

This comment has been minimized.

Copy link

juank11memphis commented Nov 1, 2018

Any idea when would this be ready? Would be awesome to have an official react-apollo hooks implementation :)

@aralroca

This comment has been minimized.

Copy link

aralroca commented Nov 1, 2018

@juank11memphis I imagine after hooks will be more stable, not alpha 😑 However, I'm also looking forward for this

@FredyC

This comment has been minimized.

Copy link

FredyC commented Nov 1, 2018

@juank11memphis @aralroca Do you realized this isn't actually about hooks? To get the most out of upcoming changes, Apollo should leverage Suspense and that works somewhat differently. Sure, you can use hooks instead of current render prop components, no harm in there and it will work (some working implementations are already done ⬆️ ). However, note that you cannot really suspend rendering with a hook. You have to handle loading states by checking loading prop.

@alan345

This comment has been minimized.

Copy link

alan345 commented Nov 2, 2018

Great article: Writing Custom React Hooks for GraphQL https://medium.com/open-graphql/react-hooks-for-graphql-3fa8ebdd6c62

@trojanowski

This comment has been minimized.

Copy link
Contributor

trojanowski commented Nov 2, 2018

Even though Suspense is not ready for data fetching, isn't it like double work to do it with hooks first and then rewrite to Suspense? I mean if you look at this video from React Conf (time included) it's kinda clear that any data fetching won't be just a simple hook. Andrew even mentions Apollo there and that it should probably wait for a proper implementation.

Isn't the base of suspense stable since React 16.6? At least the Suspense component is documented in the official React docs. I understood from the linked presentation that the unstable part is the react-cache library which will allow us to easily build suspense-compatible resources. By the base of suspense, I mean a possibility of throwing promises in the render method / functional components and React waits for them to be resolved and shows the fallback UI defined with the <Suspense /> component. @acdlite @gaearon Is there a possibility it will be changed?

@juank11memphis @aralroca Do you realized this isn't actually about hooks? To get the most out of upcoming changes, Apollo should leverage Suspense and that works somewhat differently. Sure, you can use hooks instead of current render prop components, no harm in there and it will work (some working implementations are already done ⬆️ ). However, note that you cannot really suspend rendering with a hook. You have to handle loading states by checking loading prop.

@FredyC

I don't think it's correct. You can throw a promise inside of a hook. My react-apollo-hooks seem to be suspense compatible (you actually have to use it with the <Suspense /> component). It's done in that line.

@hlehmann

This comment has been minimized.

Copy link

hlehmann commented Nov 2, 2018

@trojanowski for what I have seens with react-apollo-hooks, useRef() and other hooks are not saved when you throw the promise, breaking the app in some case.

@trojanowski

This comment has been minimized.

Copy link
Contributor

trojanowski commented Nov 2, 2018

@hlehmann Could you please create an issue at https://github.com/trojanowski/react-apollo-hooks/issues with more details?

@dai-shi

This comment has been minimized.

Copy link

dai-shi commented Nov 4, 2018

This is kind of off topic, but I've been thinking how I could use hooks without waiting for official support.
I created a small wrapper library that transforms render props to hooks. See the example below.

import { useRenderProps, wrap } from 'react-hooks-render-props';

const QUERY_POSTS = gql`                           
query queryPosts {                                        
  posts {                                                 
    id
    text
  }
}
`;

const useApolloQuery = (query) => {
  const [result] = useRenderProps(Query, { query });
  const fallbackResult = { loading: true }; // XXX this is a limitation.
  return result || fallbackResult;
};

// yeah, we can't avoid wrapping...
const PostList = wrap(() => {
  const { loading, error, data } = useApolloQuery(QUERY_POSTS);
  if (loading) return <span>Loading...</span>;
  if (error) return <span>Error: {error}</span>;
  if (!data) return <span>No Data</span>;
  return (
    <ul>
      {data.posts.map(item => <li key={String(item.id)}>{item.text}</li>)}
    </ul>
  );
});

const ADD_POST = gql`                              
mutation addPost($text: String!) {                        
  addPost(text: $text)                                    
}
`;

const useApolloMutation = (props) => {
  const results = useRenderProps(Mutation, props);
  return results;
};

const NewPost = wrap(() => {
  const [addPost] = useApolloMutation({ mutation: ADD_POST, refetchQueries: ['queryPosts'] });
  const add = (text) => {
    addPost({ variables: { text } });
  };
  return (
    <TextInput add={add} />
  );
});

The full example is in: https://github.com/dai-shi/react-hooks-render-props/tree/master/examples/03_apollo

Note that this is an experimental project for fun.

@prateekrastogi

This comment has been minimized.

Copy link

prateekrastogi commented Nov 16, 2018

Since apollo-react 2.3 released yesterday, are there any plans or timeline to give this feature request serious attention for future release?

@juank11memphis

This comment has been minimized.

Copy link

juank11memphis commented Feb 7, 2019

Hi @mbrowne

This is a real life example of what I did:

This is how I use the QueryResponseWrapper component:

const PlayingNowMovies = ({ classes }) => {
  const { data, error, loading } = useQuery(movieQueries.GET_PLAYING_NOW, {
    suspend: false,
  })
  return (
    <QueryResponseWrapper error={error} loading={loading}>
      <div className={classes.blackBox}>
        <Text as="h3" align="center" color="white">
          Playing Now
        </Text>
        <Divider className={classes.centeredDivider} />
        <div className={classes.carouselContainer} {...rest}>
          <MoviesCarousel movies={data.movies || []} />
        </div>
      </div>
    </QueryResponseWrapper>
  )
}

And this is the actual QueryResponseWrapper component:

import React from 'react'
import PropTypes from 'prop-types'

import LoadingSpinner from '../loading-spinner/LoadingSpinner'
import ErrorMessage from '../messages/ErrorMessage'

const QueryResponseWrapper = ({ children, loading, error }) => {
  if (loading) {
    return <LoadingSpinner />
  }
  if (error) {
    return (
      <ErrorMessage error={error} message="An unexpected error has occurred" />
    )
  }
  return children
}

QueryResponseWrapper.defaultProps = {
  loading: false,
  error: null,
}

QueryResponseWrapper.propTypes = {
  children: PropTypes.node.isRequired,
  loading: PropTypes.bool,
  error: PropTypes.object,
}

export default React.memo(QueryResponseWrapper)

As you can see QueryResponseWrapper has nothing to do with data, it just renders children if loading is false and there are no errors.

I hope this helped :)

@hrasoa

This comment has been minimized.

Copy link

hrasoa commented Feb 7, 2019

Maybe let’s wait for updates from the Apollo team, if there is nothing new to say. We’re all eager to use hooks and I’m sure it’s a priority for the Apollo team. Let’s not turn this issue into a Chatroom. @hwillson could you maybe lock this thread for now.

Yes please lock this thread and let's wait for the Apollo team to release their version before talking about "potential" issues.

@buzinas

This comment has been minimized.

Copy link

buzinas commented Feb 15, 2019

@hwillson Your last post was exactly one month ago, and since React Hooks were released as part of React v16.8 10 days ago, I'd like to ask: do you have any updates on this?

(No pressure, only wanted to know so I can plan our migration process).

@n1ru4l

This comment has been minimized.

Copy link

n1ru4l commented Feb 15, 2019

@buzinas Why would you put the effort into migrating everything to use the new APIs? HOCs do still work and I am pretty sure the FaaC API's will still work once the Apollo Team starts working on Hook/Suspense implementations.

@buzinas

This comment has been minimized.

Copy link

buzinas commented Feb 15, 2019

@n1ru4l I think I could have worded better. We just started using Apollo a few weeks ago, and we're still not running it in production, so for us it would be worth it to rewrite the parts we've done, because it won't be so much effort, and we'll be able to use hooks for everything new as well.

@hwillson

This comment has been minimized.

Copy link
Member

hwillson commented Feb 15, 2019

Hi all - here's where we're at:

--

The Good

  • We want Hooks support in React Apollo.
  • react-apollo-hooks is awesome, and we've talked to @trojanowski about potentially merging it into React Apollo (and he said 👍).
  • We've had many offers from the community to work on this, which is AMAZING! 🙇‍♂️

The Bad

We're still a bit concerned that working on Hooks support in React Apollo might be jumping the gun, until Suspense for data fetching is released. The final version of this will impact whatever decisions we make to adopt Hooks, and it's too early to tell how much it will impact them (but our guess is it will impact them significantly). We might be better off waiting until Suspense for data is released, as the last thing we want to do is publish a Hooks version of React Apollo that everyone starts using, only to have to change the public API again when Suspense for data hits. In the meantime, people who want Hooks support now can use react-apollo-hooks.

The Ugly

React Apollo supports HOC's via graphql, render props via Apollo Components, and soon it will support Hooks. Is this too much? Yes, there are pros/cons to using each approach, but there is a massive overlap in terms of what you can accomplish with each approach. We realize this is an issue (or maybe advantage?) across the entire React ecosystem, but one of the main things we try to do at Apollo is help give developers clear guidance on what we think is the best approach for working with GraphQL. Adding a new option (without removing an older one) might add to the confusion developers are already faced with, when it comes to picking and choosing a GraphQL data loading approach. So … if we add Hooks, should we deprecate another approach? Definitely a painful question to answer - each approach has its pros/cons, but supporting each one impacts library bundle size, maintenance overhead, documentation and knowledge sharing / training.

--

Given the above, here's our current plan:

  1. First get Apollo Client / React Apollo 2.5.0 launched. We won't be diving further into Hooks work until 2.5 is out (which is mostly dependent on docs work at this point, so hopefully it will be out next week).

  2. Once that's done, review react-apollo-hooks more extensively, planning out how it could be best integrated into react-apollo.

  3. At the same time, reach out to the React team to discuss all things Suspense for data, to get a better understanding of what the impact will be to a project like react-apollo (to see if we can glean how much of the Hooks work would have to change).

  4. Decide if we want to move forward with Hooks support, or wait for Suspense+data.

  5. If we're going ahead with Hooks support, then we'll post a design doc outlining the results of step 2 above, and if anyone in the community wants to work on it, then awesome! Otherwise we'll tackle it.

or

  1. If we're going to wait until Suspense+data is released, then we'll get react-apollo-hooks featured more prominently in the React Apollo docs, perhaps with a Hooks section and examples.
@mbrowne

This comment has been minimized.

Copy link
Contributor

mbrowne commented Feb 15, 2019

Regarding "The Ugly", that seems like a very surmountable problem, so hopefully the approach for that could be decided on already by the time Suspense is fully released. Obviously which APIs to encourage and support out of the box will need to be carefully discussed within the internal Apollo team, but (and I'm not trying to start a discussion on the details here) there are good options to provide core APIs in the main package (say, hooks and ApolloConsumer) that can be easily used to support the other APIs via separate packages. I'm sure the community would be happy to play whatever role it can here, even if that's just completing a survey and/or providing feedback on alpha/beta releases as usual.

@MrLoh

This comment has been minimized.

Copy link

MrLoh commented Feb 15, 2019

I think dropping HOC support is totally unproblematic. Our app still uses HOCs, but because some features related to error handling are not supported with HOCs (#604) I reimplemented HOCs with render Props a long time ago, just release such a tiny implementation as a separate package. Hooks will definitely replace nearly all use cases of HOCs, recompose has declared itself unneeded with hooks on the rise, so I wouldn't worry too much about that.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 15, 2019

My two cents to "The Ugly". It's definitely premature at this point, but it's good to discuss it for sure. I think that after hooks implementation is stabilized within the react-apollo V2, there could be a major V3 release that would drop the HOC. Projects using HOC would stay on V2 until they fully migrate and then they can switch to V3. This comes with the obvious downside that any bug fixes should be merged to V2 & V3 for the time being. But it feels like the most reasonable path.

Edit:
Not sure if there are people who would dislike Hooks and prefer HOC forever. I guess there is always an option to fork V2 when it stops being maintained (we are talking year or more here) and take care of it on your own. It's the cost for staying too much in the past 👯

@camjackson

This comment has been minimized.

Copy link

camjackson commented Feb 17, 2019

To state the (potentially) obvious: it would probably be a bad thing if the same release that adds hooks also removed HOCs and/or render props, because that would force people to do a big bang migration rather than a gradual one. The React team have always done an awesome job of introducing new stuff and deprecating/removing old stuff over a couple of releases, so that usually if your project runs without any warnings, it's safe to upgrade even a major version.

When it comes to Apollo, I personally am a HOC lover, and only an occasional user of render props, for reasons that aren't relevant to this thread. There's been a few mentions here of "each approach has its pros and cons", but having watched Sophie, Dan and Ryan's talks on hooks, it kind of looks to me like hooks are superior in every way to both of the other approaches.

So, maybe it's just because they're new and shiny, and I haven't gone deep enough yet to see the cons of hooks, but personally I would be totally OK with Apollo (eventually) removing support for both HOCs and render props in favour of only supporting hooks out of the box. That would definitely be a win in terms of bundle size, and simplifying code, documentation, community advice to centre around one approach. If HOC and render prop components could be supported through separate packages (e.g. react-apollo-hocs, react-apollo-components), that feels like an 'everybody-wins' result.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 17, 2019

I would really love to see some of those people who downvote dropping HOC to actually express some reasoning why they need it: @brainkim @rj254 @volkanunsal @remi2j. Is it purely an annoyance of refactoring old code or is there something else?

it kind of looks to me like hooks are superior in every way to both of the other approaches.

There is one weak spot - conditional rendering. I know, there is a skip option, but in some cases it's not optimal. There might be some variables which you cannot fulfill at the time (you get them from eg. another query) and you would need to add weird conditions there. The Query prop kinda wins this one.

If HOC and render prop components could be supported through separate packages (e.g. react-apollo-hocs, react-apollo-components), that feels like an 'everybody-wins' result.

It's a brave idea, but I don't think anybody would have a will power to maintain it, except perhaps those people who would need it and that's kinda the "fork idea" I was talking about before.

@volkanunsal

This comment has been minimized.

Copy link

volkanunsal commented Feb 17, 2019

Since you asked @FredyC. Deprecating HOC is not just an "annoyance." It's a considerable undertaking for those of us who are fully invested in the HOC pattern. It requires rewriting thousands of lines of code across several projects, and it directly affects the bottomline of our business.

Secondly, conditional rendering is an issue, like you mentioned above. We have several places where this pattern is used.

Third, there are scenarios where using HOC is more advantageous than using hooks. It is a common pattern to drill into a query result and store only a small part of it in a prop above your render component. You cannot do this if you're using hooks.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 17, 2019

@volkanunsal Thank you for your bravery :)

Deprecating HOC is not just an "annoyance." It's a considerable undertaking for those of us who are fully invested in the HOC pattern. It requires rewriting thousands of lines of code across several projects, and it directly affects the bottomline of our business.

Alright, fair enough. Do you have for example some periodic library updates? I mean unless something really big happens in GraphQL world, the current version of react-apollo is just fine for you, right? Now imagine if there would be V2 which would still keep HOC + Render prop and add Hooks so you can choose any of that. For others, there would V3 which would drop HOC. Would you be forced to use V3 ever? I don't think so :)

It is a common pattern to drill into a query result and store only a small part of it in a prop above your render component. You cannot do this if you're using hooks.

That's far from the truth. On the contrary, Hooks allows much better composition than HOC could ever dream of. Hooks are not forcing you to punch everything into one big component. It's only a matter of open mind and a bit of a fantasy to see some really amazing patterns.

@volkanunsal

This comment has been minimized.

Copy link

volkanunsal commented Feb 17, 2019

That's far from the truth. On the contrary, Hooks allows much better composition than HOC could ever dream of.

No, it's not. How would you cache intermediate artefacts of a query result using hooks? This kind of cache is absolutely essential if you're doing complex transformations with your query results. Also –– and I just remembered this –– since the data object turns into an empty object when Apollo is loading new data (at least that is what react-apollo-hooks does with the data object), you need to store the results somewhere as Apollo fetches new data if you don't want to put your component into "Loading" state or have it completely broken. That is why I haven't been able to use Apollo hooks anywhere except in trivial components that don't seem too badly affected by the flakiness that occurs because of this.

I'd be happy to gradually adopt hooks into my codebase, but I wouldn't want to give up an old technique that is still more complete than a new one, no matter how amazing you say it is. From the looks of it, it doesn't do everything HOC can do right now.

@volkanunsal

This comment has been minimized.

Copy link

volkanunsal commented Feb 17, 2019

@FredyC I rewrote one of my components using Hooks and HOC to think more about this. I still think the second one is more elegant, but I admit using the state hook can address the intermediate data issue I raised above.

If the data object wasn't emptied out during data fetch, I might be able to get rid of the NonEmptyObject checks, too. Then, the hook approach would be just as elegant as the HOC approach.

Hooks

const DataList = ((props: { companyId: ID, filters: string[] }) => {
  const opts = {
    suspend: false,
    variables: {
      filters: props.filters,
      companyId: props.companyId,
    },
  };
  const { data, loading } = useQuery(FavoritesQueryDef, opts);
  const [newData, setData] = useState(data);

  if (data !== newData && NonEmptyObject.is(data)) {
    setData(data);
  }
  const hasData = NonEmptyObject.is(newData);

  // ... 
  
  return (
    <React.Fragment>
      {items}
      <Loading show={!hasData && loading} />
    </React.Fragment>
  )
}

HOC

const DataList = graphql(FavoritesQueryDef)(props => {
  const { data, loading } = props

  // ... 
  
  return (
    <React.Fragment>
      {items}
      <Loading show={loading} />
    </React.Fragment>
  )
})
@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 17, 2019

@volkanunsal

If the data object wasn't emptied out during data fetch, I might be able to get rid of the NonEmptyObject checks, too. Then, the hook approach would be just as elegant as the HOC approach.

I have noticed this as well. There are same underlying concepts inside the HOC and Hooks, so unless HOC is doing some weird voodoo magic, I am fairly certain it's just a bug in Hooks version, but had no capacity to pay attention to it.

Also –– and I just remembered this –– since the data object turns into an empty object when Apollo is loading new data (at least that is what react-apollo-hooks does with the data object), you need to store the results somewhere

You mean when variables change and query is re-run? Haven't noticed that really, but it's definitely super easy to abstract such behavior into custom hook if you need a previous result available.


In the HOC you left out types and variables for a brevity? :) Especially those variables are very awkward with HOC since you cannot just access props within a closure (yay Hooks). And types to be defined with HOC instead of with component is also strange. Let's have a look at a real example, shall we? :)

// had to separate it, as it would be too unreadable mixed with the component
const decorate = graphql<{ companyId: ID, filters: string[] }>(
  FavoritesQueryDef, {
    options: props => ({
      variables: {
        filters: props.filters,
        companyId: props.companyId,
      }
    })
  }
)
const DataList = decorate((props => {
  const { data, loading } = props

  // ... 
  
  return (
    <React.Fragment>
      {items}
      <Loading show={loading} />
    </React.Fragment>
  )
})

How would you cache intermediate artefacts of a query result using hooks? This kind of cache is absolutely essential if you're doing complex transformations with your query results.

Not sure what you mean by that, like a memoization? What about useMemo? You could have seriously dope custom hooks that would accept selectors to make that happen. Or any other kind of cache you would like. While with HOC it's rather hard to make a custom one, Hooks are just functions so you can use whatever you like in them.

I'd be happy to gradually adopt hooks into my codebase, but I wouldn't want to give up an old technique that is still more complete than a new one, no matter how amazing you say it is. From the looks of it, it doesn't do everything HOC can do right now.

Sure, Hooks are still fresh out of the oven and more patterns will certainly emerge over time. And fair about of bug will be squashed as well :) HOC had more time to grow, so it feels much more polished for sure.

@volkanunsal

This comment has been minimized.

Copy link

volkanunsal commented Feb 17, 2019

Not sure what you mean by that, like a memoization? What about useMemo?

useMemo is something like a pure component, right? It's not exactly what I had in mind. I meant saving transformed artefacts. I think state hooks will do a good enough job for that.

I rewrote useQuery with a custom hook to eliminate the NonEmptyObject checks in the render component. We can apply the same technique for transformed artifacts, as well. It was easier than I thought it would be. Still not as elegant as HOC, but it's definitely in the same ballpark with HOC now.

function useCachedQuery(query, opts) {
  const { data, error, loading } = useQuery(query, opts);
  const [newData, setData] = useState(data);
  if (data !== newData && NonEmptyObject.is(data)) {
    setData(data);
  }
  return { data: newData, error, loading };
}

const DataList = ((props: { companyId: ID, filters: string[] }) => {
  const opts = {
    suspend: false,
    variables: props,
  };
  const { data, loading } = useCachedQuery(FavoritesQueryDef, opts);
  const hasData = NonEmptyObject.is(newData);

  // ... 
  
  return (
    <React.Fragment>
      {items}
      <Loading show={!hasData && loading} />
    </React.Fragment>
  )
}

I can see that hooks aren't much worse than HOCs in terms of storing intermediate query artifacts, like I thought. Thanks for the nice discussion @FredyC.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 17, 2019

I hope that other people can see what you have just learned 👍 I believe this is kinda a root of the problem. People are used in some ways and they are content about them because there was nothing better at that time. Why would they bother with some Hooks, right? There is an obvious barrier which is hard to beat for sure, but it can pay back (with dividends) 💰

useMemo is something like a pure component, right? It's not exactly what I had in mind. I meant saving transformed artefacts. I think state hooks will do a good enough job for that.

No, that's React.memo you are talking about. The useMemo is really for memoization purposes.

Btw, tiny nit :) No need to repeat that hasData check in every component, right? You could even "merge" it with loading and hide that away completely. With HOC something like that is much harder to do as you need to add another HOC in middle to modify props.

 const { data, loading, hasData } = useCachedQuery(FavoritesQueryDef, opts);

Best part of about programming is the amount of code you delete!

@mbrowne

This comment has been minimized.

Copy link
Contributor

mbrowne commented Feb 18, 2019

As with any new, shiny technology, I think it's important to be wary of too much "silver bullet" thinking. But having said that, I think hooks are a particularly good fit for graphql/Apollo. I doubt that direct usage will be superior 100% of the time, but hooks could still be the underlying implementation even in those cases. @FredyC suggested that extracting HOCs and render props into a separate package might lead to a situation where they're not sufficiently maintained, but if you think about how hooks could be used to implement those same APIs, there's really not that much to maintain...for example, the Query component could be implemented as simply as:

import { useQuery } from 'react-apollo-hooks'

export default function Query({ query, children, ...otherProps }) {
    return children(useQuery(query, { ...otherProps, suspend: false }))
}

And you can even still use it from class components! ...if you need to maintain components you don't intend to migrate to function components (assuming you intend to migrate at all).

To be a bit more clear about where direct hook usage might not be the best, I'm still not convinced that the QueryLoader use case I mentioned above is improved in any way by the hook implementation suggested by @juank11memphis (much as I appreciate his feedback, the render prop solution still seems better to me from the usage perspective). That use case might always be handled in app code (as it is now in my app) rather than being supported by react-apollo directly, so I don't think this thread is the right place to discuss the tradeoffs (maybe we can discuss it elsewhere). But just to illustrate my above point, that use case too could be implemented by hooks behind the scenes and the external API could still use a render prop as my current QueryLoader component is doing:

function Demo() {
    return (
        <QueryLoader query={MY_QUERY} variables={...}>
            {({ data }) => (
                <div>
                    {data.foo}
                </div>
            )}
        </QueryLoader>
    )
}

I'm actually less concerned about the conditional rendering use case. I might be missing something, but I think that could be easily handled by an alternative mode for useQuery() (perhaps activated by a simple boolean parameter) that returns a function that executes the query rather than executing the query immediately—similar to how you can call react-apollo-hooks's useMutation() to return a mutation function that you call later. So something like:

const runQuery = useQuery(MY_QUERY, { curry: true })
...
    // later, possibly inside some condition...
    <Suspense fallback={<div>Loading...</div>}>
        {/* Passing a function to React.createElement() might not be the best idea here;
            just keeping the example concise */}
        {React.createElement(() => {
            const { data, error } = runQuery({ variables: ... })
            ...
        })}
    </Suspense>
@Nayni

This comment has been minimized.

Copy link

Nayni commented Feb 18, 2019

As with everyone in this thread I've been very interested to know what the future of React Apollo holds regarding hooks and also Suspense for the future.

I agree the remarks of The Ugly and would love to see Apollo come up with a go-to solution for the broader audience. It would be great that the usage of React-Apollo leads you to the most acceptable and clear solution to add data fetching in your app. I find this especially important for data fetching because it is the main portion of state of any medium to large size application so getting this right is a big deal.

With that said, and looking at how Hooks are the new solution forward I think it's more than logical to focus this solution into that direction, mainly because Hooks seem to be the way the React team wants to move forward as well.

I personally see this like how Render Props came into the eco-system, the only big different with Hooks and Render Props are that Hooks are an official feature React itself, while Render Props was a powerful pattern that emerged from the community.
Even though Hooks are still young they do seem to solve the majority of problems and it also seems that the React team is pushing Hooks to be the solution to the majority of state management. I also notice that for the majority of problems you can solve with Render Props or HoC's there is a Hooks variant you can write, and most importantly if you can implement it with Hooks you can wrap it in the previous patterns (being Render Prop or HoC).

I agree with @camjackson that the migration is a point to be discussed and taken carefully but what I would love to see is for React Apollo to be pushing the standardized solution which I believe will be Hooks but then also offer me quick bindings to convert Hook implementations into HoC's or Render Props. @mbrowne shows some relevant use-cases for this which. I think here React Apollo could come with additional packages or documentation support to help users implement such use-cases (I'm thinking something like react-apollo-hoc or react-apollo-renderprop which could be tiny packages that wrap the official Hook implementation into a consumable HoC or RenderProp component.

We currently have a code base that is full of Higher order components and the re-compose pattern. We are on the fence to move to Hooks because a lot of this is decided by having it baked into the data fetching layer which at this point is to either wrap it ourselves or resort to react-apollo-hooks, but this has the risk that we'll have to re-do our work once React-Apollo ships with its own official hooks and we'd like to avoid doing a double refactor.

@jamuhl

This comment has been minimized.

Copy link

jamuhl commented Feb 19, 2019

I do like to share some insights on how we handled above the good / the ugly in i18next.

We took the chance to make a full rewrite of our react integration for the i18next i18n lib. For us that had big impact in reducing module size and clean up a lot of stuff.

Why had we similar "ugly" problem? i18next supports adding "backend" plugins to load translation files from the backend server -> so our hooks might be in state "loading/notReady" or "loaded/ready" but we decided to use Suspense because Suspense itself is defined and works very well -> throw a promise which gets catched higher up the JSX tree and suspense rendering until that suspense gets resolved: https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L76

Why do we feel save to do so:

  • Suspense itself is ready to use - the data-fetcher is not (but data-fetcher is more a combination of cache and suspense
  • i18next takes care for only requesting a translation file once, caching of it -> so we do not depend on the data-fetcher in future.
  • the benefit of not having to handle all the "loading" flags in the components but in the suspense is a huge benefit
  • for defensive users like to avoid suspense we allow setting a global flag to fallback to the loading state

See also the same discussion we had about using suspense and breaking the rules of hooks:

-> i18next/react-i18next#735
-> React issue about throwing a Suspense from Hook: facebook/react#14848

the hook enabled us to provide other usage options like HOC and render prop by reusing the hook:

This is just what we did and the case for apollo might be a lot different. So just take this as is.

D1no added a commit to D1no/coinglaze that referenced this issue Feb 19, 2019

@sessionboy

This comment has been minimized.

Copy link

sessionboy commented Feb 20, 2019

Which version is expected to support the react hook ?

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 20, 2019

@sessionboy

This comment has been minimized.

Copy link

sessionboy commented Feb 20, 2019

@FredyC #2539. It's here. But it seems to be still under discussion.

@FredyC

This comment has been minimized.

Copy link

FredyC commented Feb 20, 2019

@sessionboy But it kinda answers your question, right? Since no one knows, then no one can give you a better answer.

@DAB0mB

This comment has been minimized.

Copy link

DAB0mB commented Feb 20, 2019

I would like to share with everyone my experience with Apollo-GraphQL and React-hooks while building a WhatsApp-Clone. So far when I wanted to run GraphQL operations I had to implement dedicated React.Elements within the virtual DOM tree, which from an architectural perspective never really made sense to me. The virtual DOM's purpose is to build the view by binding data-models into a template, and so by adding data-fetching into the mix you basically abuse it. In addition, having to use more than one GraphQL-operation for a React.Component can cause these extra indentations which basically brings us back to the issue with ES5 where we had a callback hell; this is why they invented Promises and later on async/await.

Because of that, I was fairly excited when I first noticed @trojanowski's react-apollo-hooks package. Not only it solves all the issues above, but it also supports React.Suspense for data-fetching. This way you can control which nodes up the virtual DOM tree are gonna remain visible while waiting for the data to fetch, regardless of the implementation of the fetching React.Component. The clear advantage of using React.Suspense over not using it, was that I didn't have to handle multiple loading flags; It saved a lot of time and potential errors. Sure there are some limitations to that, such as concurrent data-fetching (something like Promise.all), but still, it all made a lot of sense and it just worked. I would love to see Apollo heading that direction.

import gql from 'graphql-tag';
import { useQuery } from 'react-apollo-hooks';

const GET_DOGS = gql`
  {
    dogs {
      id
      breed
    }
  }
`;

const GET_CATS = gql`
  {
    cats {
      id
      breed
    }
  }
`;

const Animals = () => {
  // No extra indentions, easy to read and maintain
  const { data: { dogs } } = useQuery(GET_DOGS);
  const { data: { cats } } = useQuery(GET_CATS);

  // return ...
};
@mbrowne

This comment has been minimized.

Copy link
Contributor

mbrowne commented Feb 20, 2019

Good point about the virtual DOM—that's one of the benefits the React team cites for hooks as well. The principle I have gathered from my understanding so far is, "hooks for shared logic, components for rendering". Data-fetching certainly falls in the "shared logic" category. (I'll have to think more about my QueryLoader component and whether or not it actually justifies adding another element to the virtual DOM. It does do rendering—the loader component, error message, or content as the case may be.)

Side-note about concurrent data-fetching and suspense: you've probably aware of this already, but I think ConcurrentMode could help with this: https://reactjs.org/blog/2018/11/27/react-16-roadmap.html#react-16x-q2-2019-the-one-with-concurrent-mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment