Skip to content
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

Replace custom ApolloContext with official one #131

Closed
FezVrasta opened this issue Apr 16, 2019 · 15 comments
Closed

Replace custom ApolloContext with official one #131

FezVrasta opened this issue Apr 16, 2019 · 15 comments

Comments

@FezVrasta
Copy link

Starting from [email protected] we have access to the ApolloContext (https://unpkg.com/[email protected]/ApolloContext.js).

This would enable this library to get rid of the custom ApolloProvider and ApolloContext and just rely on react-apollo

@trojanowski
Copy link
Owner

It's not exposed in the cjs and esm builds: https://unpkg.com/[email protected]/react-apollo.cjs.js.

@FezVrasta FezVrasta reopened this Apr 21, 2019
@FezVrasta
Copy link
Author

FezVrasta commented Apr 22, 2019

[email protected] exposes its context now!

I'd be willing to prepare a PR but I'm not sure how would you like to proceed. Introduce a breaking change and completely deprecate the react-apollo-hooks context, or try to make both contexts work?

I would prefer to remove support for the custom context, but I see how people may like to use react-apollo-hooks without having to rely on react-apollo at all. But, considering this is just a temporary library while we wait for official support, I think it would make sense anyways.

@trojanowski what's your opinion?

@danielkcz
Copy link
Contributor

danielkcz commented Apr 22, 2019

Well, I think we can just utilize ApolloContext instead of a custom one. And the provider from this package should yield a deprecation warning and use ApolloContext.Provider internally. That won't break anything AFAIK.

@FezVrasta
Copy link
Author

FezVrasta commented Apr 22, 2019

So:

  1. Make react-apollo-hooks' ApolloProvider point to react-apollo context
  2. Add react-apollo@^2.5.5 as peer dependency
  3. Add warning when one tries to use the old ApolloProvider
  4. profit?

(isn't the peerDependency a breaking change nonetheless?)

@danielkcz
Copy link
Contributor

Actually, I've managed to migrate the app to hooks so I've dropped the react-apollo from dependencies altogether. Hm, that's bummer. I don't want to add it again just because of the context :/

I guess it would need to safely detect if react-apollo is available and fall back to custom context if not.

@FezVrasta
Copy link
Author

Doing import { ApolloContext } from 'react-apollo' will throw an error in webpack/bundler-of-choice if the dependency is not available. How can you import only if the package is available?

@danielkcz
Copy link
Contributor

Yea, that's the tricky part. Sadly I am not aware of any safe way of doing that. I do remember seeing a trick like this but never tried if it actually works.

const r = require
try {
  r('react-apollo')
} catch (e) {
  // not available
}

Perhaps it's not such a good idea doing this after all. I mean this package eventually will be merged to the official one. We just need to be patient.

@FezVrasta
Copy link
Author

FezVrasta commented Apr 22, 2019

Alternatively we could make react-apollo a dependency, this would make the whole thing transparent to the consumer. (you don't need to have react-apollo in your package.json)

@danielkcz
Copy link
Contributor

I suppose that would work, but it feels like excessive dependency just to be there just for such a little piece of information. I don't know, I feel it's not such a big deal in the end to have two providers. Initially, I made myself this piece of code to abstract such a detail away.

import { ApolloProvider as ApolloProviderHooked, useApolloClient } from '@speedlo/react-apollo-hooks'
import React from 'react'
import { ApolloProvider as ApolloProviderOrig, ApolloProviderProps } from 'react-apollo'

export function ApolloProvider<TCache>({
  client,
  children,
}: ApolloProviderProps<TCache>) {
  return (
    <ApolloProviderOrig<TCache> client={client}>
      <ApolloProviderHooked<TCache> client={client}>
        {children}
      </ApolloProviderHooked>
    </ApolloProviderOrig>
  )
}

export { useApolloClient }

@FezVrasta
Copy link
Author

FezVrasta commented Apr 22, 2019

It's not a big deal, but people get easily scared by it. In my team I received concerns from several team members because of that provider, they think this library tries to replace Apollo completely.

I know it's not this library's fault, but it's something that could be easily avoided if the README told the users this library can be used in their current react-apollo applications by just importing and using the hooks exposed by it without additional setup.

@danielkcz
Copy link
Contributor

Honestly, if someone is not using react-apollo-hooks yet, they are most likely waiting for it to stabilize and become part of the react-apollo. People who already embraced hooks have two providers anyway.

Let's some other people chime in on this or you can prepare some preliminary PR and we can go from there.

@trojanowski
Copy link
Owner

I agree with @FredyC. It's often possible to replace all HOCs / render props with hooks (or just start with hooks and don't use alternative APIs at all, e.g. in new projects). And after that, it's easier to install only one library, instead of two. And if you want to use react-apollo and react-apollo-hooks together, the integration is quite simple. @FezVrasta if you don't agree with that maybe a better idea will be to improve the documentation?

@FezVrasta
Copy link
Author

I think the documentation is fine, it's just the developer experience that lacks

@dantman
Copy link

dantman commented Apr 25, 2019

I use both react-apollo and react-apollo-hooks, I probably won't ever just use react-apollo-hooks on it's own. So a dependency on react-apollo to reduce the provider duplication wouldn't hurt me.

For context, so far I am using <Query> for most of my querying and useMutation for most of my mutations. useQuery cannot be done conditionally (e.g. you have a query that requires account.id but need a conditional guard against account === undefined). And <Query> can also be done deeply without re-rendering the whole containing component for just a partial tree change.

@danielkcz
Copy link
Contributor

@dantman For conditional querying, I made my own Query based on useQuery. Sometimes I opt for using skip in simple cases and when I need to access data in the body of a function. Render prop is just awkward with hooks around :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants