-
Notifications
You must be signed in to change notification settings - Fork 793
Conversation
ApolloProvider wrapper Component
Enable Tests, and work around odd TS.
Apollo Provider tests and touch up
@@ -0,0 +1,60 @@ | |||
/// <reference path="../typings/main.d.ts" /> | |||
/* tslint:disable:no-unused-variable */ | |||
import * as React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird errors in compile. Anyone know what this is all about? Had to do this to appease Provider
Yeah I think something as simple as changing the export name can be good to avoid confusion between the two. |
@abhiaiyer91 I'm not a fan of having to pass a store. It locks us into redux (which I like redux fwiw) and exposing an internal part of the client (that is exposed for advanced reasons) adds a barrier to usage in my opinion. |
@jbaxleyiii I agree with you, but this component is strictly for Redux implementation no? Unless we want to use this at the forefront, then what I'll do is make it behave like the current react-apollo |
@abhiaiyer91 ah I see, this wouldnt be the most commonly used provider. One thing I was wondering about is composing with react-redux provider for ours and exposing that composition method so people already using provider wouldn't need to replace, just compose the Apollo addition That way there is only one provider for clarity but we still get the benefits mentioned above. Thoughts? |
I am down with that @jbaxleyiii. That will be a lot cleaner. What do you think @stubailo I'll close this PR and make that happen. |
@abhiaiyer91 awesome! Thanks! |
Great idea! But I still think we could rename |
Yeah I'll do that in another PR |
In reference to this issue #142
This PR is the first step of providing Apollo users a direct integration with the Redux library and set of existing tools.
What are we shipping?
I think to provide a seamless integration into current Redux workflows, exporting an
ApolloProvider
allows us to maintain the Apollo integration a layer aboveReactRedux
'sProvider
implementation.Usage
To note:
store
is a required propBenefits
On the surface this change doesn't seem like a big deal. And really it isn't far off then what
react-apollo
ships, but:react-apollo
to gain upstream changes to theProvider
component fromreact-redux
. 2. helps alleviate confusion between large apps that may be fragmented, meaning they don't have clear component trees e.g. multipleProviders
who may be confused whatProvider
they are actually using. People don't read 馃毃