Refactor ApolloProvider/ApolloConsumer implementation to use React 16.3 context API

[vovacodes]

The old way of passing the context still exists side-by-side with the new way to allow gradual migration of <Query>, <Mutation> and other modules that access the context in the "old" way.
So this change is completely backwards compatible

This should unlock experiments with React Hooks version of react-apollo API (#2539) because if you want to access the context(client) in a hook, the context has to be exposed using the "new" context API

closes #1937

[apollo-cla]

@wzrdzl: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[janek26]

CI for Preact will fail until developit/preact#963 is resolved

[vovacodes]

@jbaxleyiii what shall we do about Preact? Wait until developit/preact/pull/963 is merged or add a some handling for the case when createContext is not a function?

[stnwk]

@janek26 @jbaxleyiii I don't think this whole PR should be postponed just because auf preact-compatability. The blocking PR you mentioned preactjs/preact#963 has had no activity since July..

Preact and React versions will probably diverge even more in the future so we might need a different solution here doesn't block the other one :/

[danielkcz]

Would love to see this merged as well. Right now it's basically necessary to have a custom Provider in the tree if I want to consume the ApolloClient eg. by hooks.

I agree that waiting for Preact is not feasible in a long run with all that sweetness of Suspense and Hooks, it's unlikely they will be able to catch up soon enough. Perhaps preact-apollo could be made out of the current master and let it go its own path?

[prateekrastogi]

maybe creating a different branch and publishing the alpha-version of this package supporting react-hooks and suspense will satisfy all the stakeholders, and consequently save the time in the main release when preact supports new context api? We can't wait every time on two upstream repos especially when other projects including react actively publish alpha packages. Moreover, the preact merge is dependent on reactjs/rfcs#2 to be approved. And, we all have past experiences of how slow such processes can be. Even in case of new context API, react implemented it in 16.3 while the proposal is still in process, and react 16.3 was released on March 30 2018 and it is November now.

[arianon]

Why not just use the create-react-context polyfill? It works well and the preact tests pass when using it.

[danielkcz]

@arianon That could be also a way, but only if it can be tree shaken well enough. It's not a good idea to bloat build just to support some React clone :)

[arianon]

@FredyC It's small enough (1.78kB min+gzip) to keep react-apollo under its current bundlesize target (11.5kB), so I wouldn't worry about that.

[prateekrastogi]

Maybe bloating on alpha version will be an acceptable tradeoff? 😬

[danielkcz]

Maybe bloating on alpha version will be an acceptable tradeoff? 😬

To me, an acceptable tradeoff is not to support Preact in alpha version if they do not care about it (assuming from 4 months old issue).

[prateekrastogi]

@FredyC i agree with you. But, i guess it's more of business issue. As big enterprise client might be using preact in production and supporting them is essential priority for Apollo. Apollo revenue strategy seems to be focused on high traffic enterprise client. But, sacrificing DX which brings enterprise customers is somewhat awkward. Although, i am a fan of and use Apollo server and react Apollo for the quality DX that Apollo stack provides. But, may be sending this feature request from our company email ids' can be helpful 🤐

[benjamn]

Thanks for working on this @wzrdzl!

The meat of this PR is solid, but it would be easier to accept if it didn't involve adding a new npm dependency (memoize-one). If it's really important to avoid creating more than one child context object per client, perhaps there's a more direct way to implement that behavior, like sticking a special property on the client, which is something we're already doing with the operations map? Alternatively, I think I would be fine with this PR without any memoization.

[benjamn]

As for Preact, perhaps there's a way for this code to detect if Preact is being used (e.g. React.createContext is not defined), and instead behave like it did before, without returning ApolloContext.Provider or ApolloContext.Consumer?

[autarc]

Is there an update about the current state of the PR @wzrdzl regarding @benjamn concerns to avoid the additional dependency? Otherwise I would like to take a look to ensure context migration works in StrictMode 😄

[liamross]

A note on memoize-one, the actual src file is tiny so if the concern is to avoid another dependency I'm sure the functionality can be extracted from it with proper attribution (or, alternatively, if the concern is size perhaps there is no need to be concerned).

https://github.com/alexreardon/memoize-one/blob/master/src/index.js

(@benjamn @wzrdzl)

[hwillson]

I agree with @benjamn regarding the removal of memoization, and until Preact X is launched (which does support createContext), I think leveraging the create-react-context polyfill is a good workaround (since it only adds about 50 bytes to the bundle, after gzip).

These changes have been made, so we should be close to getting this merged. Let me know if anyone objects - thanks!

[hwillson]

I think we're all set here - LGTM!

[mattfwood]

Wouldn’t we need to also export ApolloContext in index.ts? If you try to useContext with a consumer or provider, it doesn’t work properly

[danielkcz]

@mattfwood The useApolloClient (or similar) should be exposed instead imo. But you have surely nailed that it has eluded in this PR.

Edit: Made the PR ... #2872

[leebenson]

Just a heads-up: after bumping to 2.5.3 which uses this, I started to get console errors about fbjs/lib/warning being missing.

This seems to have come from create-react-context, which is odd since fbjs is listed as a dep (although pegged to 0.8.0); npm i fbjs (1.0.0) fixed it.

Just an FYI for anyone else stuck on this.

[MrLoh]

@hwillson since #2872 will only be part of 3.0, couldn't the ApolloContext just be exposed directly, then everyone who want's this hook can use it by doing useContext(ApolloContext) but there is no requirement for anything else?

[notgiorgi]

@hwillson since #2872 will only be part of 3.0, couldn't the ApolloContext just be exposed directly, then everyone who want's this hook can use it by doing useContext(ApolloContext) but there is no requirement for anything else?

As it's not gonna be exposed, here's the workaround I came up with: #2872 (comment)