ApolloProvider #36

abhiaiyer91 · 2016-04-29T21:40:26Z

After much iteration, the best way to interact with the redux provider is through a wrapper component. We ended up changing the namespace of Provider to ApolloProvider, and uses react-redux provider. Added some more tests too.

ApolloProvider wrapper Component

Enable Tests, and work around odd TS.

Apollo Provider tests and touch up

Compose an ApolloProvider

add test for store context

abhiaiyer91 · 2016-04-29T22:19:35Z

Man, Test coverage went down lol. wth

…lloProvider

Add some more tests

abhiaiyer91 · 2016-04-29T22:28:41Z

How shall the version bump go?

stubailo · 2016-04-29T22:29:05Z

@abhiaiyer91 sigh, coveralls is really unreliable.

Let's bump the version after merging!

abhiaiyer91 · 2016-04-29T22:29:43Z

@stubailo I made a mistake and also grouped the namespace change as well!

abhiaiyer91 · 2016-04-29T22:30:35Z

@stubailo @jbaxleyiii sorry for mixing in the namespace change with this body of work. I can reopen 2 PRs if necessary! Let me know!

change Provider namespace to ApolloProvider

abhiaiyer91 · 2016-04-29T22:33:01Z

We should also rewrite the docs and example apps with new Namespace if merged. And add a section for the composable provider. Would you like an issue opened in the docs or here in react-apollo

jbaxleyiii · 2016-04-29T22:38:19Z

The docs would be great and mention this PR

stubailo · 2016-04-29T22:41:50Z

NP, one PR is fine!

abhiaiyer91 · 2016-04-29T22:48:16Z

Alright issues made. Whenever you guys get a chance to give some feedback, that'd be great! Once merged @stubailo assign the issue in the docs to me!

abhiaiyer91 · 2016-04-29T22:48:54Z

src/composeApolloProvider.tsx

@@ -0,0 +1,72 @@
+/// <reference path="../typings/main.d.ts" />
+/* tslint:disable:no-unused-variable */


this should be removed.

jbaxleyiii · 2016-04-29T22:51:00Z

👍 I'll take a look tonight

abhiaiyer91 · 2016-05-02T21:10:25Z

@jbaxleyiii @stubailo show this PR some love! haha

stubailo · 2016-05-02T21:14:22Z

src/Provider.tsx

@@ -17,7 +17,7 @@ export declare interface ProviderProps {
  client: ApolloClient;
 }

-export default class Provider extends Component<ProviderProps, any> {
+export default class ApolloProvider extends Component<ProviderProps, any> {


Rename the file, as well

Also, shouldn't ApolloProvider just use composeApolloProvider under the hood now?

Yeah those are the kinda questions I wanted to talk through. Since we added some duplication with this function.

I think ApolloProvider should be shipped using composeApolloProvider under the hood.

stubailo · 2016-05-02T21:20:59Z

src/composeApolloProvider.tsx

+    client: ApolloClient;
+}
+
+export default function composeApolloProvider<T>(Provider: React.ComponentClass<any>): any {


Is this function better than simply having ApolloProvider always use Provider as the implementation? As long as it's a peer dependency, the developer can choose whatever version of the Redux provider they feel like.

I guess I feel like it just adds an extra line of code, but doesn't really add much clarity since you still don't know what it's doing with the Provider. If we wanted people to have the ultimate clarity about what the provider is doing we could just ask them to use two providers:

<Provider> <ApolloProvider> ...

Actually I think now that we are actually using Redux under the hood for the provider, we should just recommend people use this directly. Now that my feet are more wet into Apollo, I think using the ApolloProvider directly with this implementation makes more sense, than composing it yourself in app.

abhiaiyer91 · 2016-05-02T21:27:36Z

Thanks for the feedback, will iterate again!

stubailo · 2016-05-02T21:29:43Z

Thanks - I'm going to try to get you that architectural overview ASAP

abhiaiyer91 · 2016-05-03T04:22:48Z

@stubailo @jbaxleyiii i think this is good to go!

stubailo · 2016-05-03T04:27:59Z

test/ApolloProivder.tsx

@@ -0,0 +1,97 @@
+/// <reference path="../typings/main.d.ts" />


Typo in this file name! (ApolloProivder)

shit man sorry!

jbaxleyiii · 2016-05-03T04:28:19Z

@abhiaiyer91 @stubailo I'm good with this! We will need to update docs and the Changelog. I'll publish it as v0.3.0 after we merge

stubailo · 2016-05-03T04:29:40Z

I think we need a corresponding docs PR before merging.

stubailo · 2016-05-03T04:31:07Z

Also, this PR changes everything from 2-space tabs to 4-space tabs, which isn't great.

jbaxleyiii · 2016-05-03T04:32:10Z

I didn't catch that. I'm surprised the linter didnt bug out. Let's fix that

abhiaiyer91 · 2016-05-03T04:33:07Z

I just noticed. WebStorm messing with me...

abhiaiyer91 · 2016-05-03T04:33:12Z

Let me fix!

stubailo · 2016-05-03T04:34:12Z

I think about a year ago all of the linters collectively gave up on linting whitespace since there are so many edge cases. but maybe it's possible.

I don't think any linter can save us from inspecting the code :]

stubailo · 2016-05-03T04:42:57Z

src/ApolloProvider.tsx

@@ -33,8 +38,8 @@ export default class Provider extends Component<ProviderProps, any> {
    client: PropTypes.object.isRequired,
  };

-  public store: Store<any>;
-  public client: ApolloClient;
+  public store:Store<any>;


This should have a space, I thought TSLint enforced this. @abhiaiyer91 you might want to set up TSLint in your editor.

abhiaiyer91 · 2016-05-03T04:46:12Z

Alright Editor is all setup! Hahah my bad. I hit the "Typescript" button in WebStorm, turns out my settings were F'd up.

stubailo · 2016-05-03T04:47:10Z

Feel free to commit in a WebStorm config! We have one for vscode in the repo.

stubailo · 2016-05-03T04:48:34Z

Looks great to me!

Last step is the docs PR, then we can merge and publish!

abhiaiyer91 · 2016-05-03T04:51:58Z

Okay I'll get the docs PR together now.

abhiaiyer91 · 2016-05-03T05:54:53Z

Docs PR https://github.com/apollostack/docs/pull/61

stubailo · 2016-05-03T16:44:56Z

OK, up to @jbaxleyiii to merge and publish this and the docs change at the same time!

abhiaiyer91 added 11 commits April 28, 2016 08:07

ApolloProvider wrapper Component

9c9a380

Merge pull request #1 from abhiaiyer91/ApolloProvider

b2a0179

ApolloProvider wrapper Component

Merge pull request #2 from abhiaiyer91/ApolloProvider

37d1022

Enable Tests, and work around odd TS.

Apollo Provider updates

886b4fb

Merge pull request #3 from abhiaiyer91/ApolloProvider

a2c62ff

Apollo Provider tests and touch up

Finish up tests and ship it

04a7f17

ApolloProvider -> composeApolloProvider

91aed15

Merge pull request #4 from abhiaiyer91/ApolloProvider

90b117c

Compose an ApolloProvider

add store context

f2581fd

Merge pull request #5 from abhiaiyer91/ApolloProvider

8cb4fbf

add test for store context

clean up style

e682175

abhiaiyer91 added 3 commits April 29, 2016 15:26

more tests for the compose fn

720a29f

Merge branch 'master' of github.com:abhiaiyer91/react-apollo into Apo…

fae85b5

…lloProvider

Merge pull request #6 from abhiaiyer91/ApolloProvider

1e4f82c

Add some more tests

abhiaiyer91 added 2 commits April 29, 2016 15:31

change Provider namespace to ApolloProvider

cf87a38

Merge pull request #7 from abhiaiyer91/ApolloProviderNamespace

bc3f728

change Provider namespace to ApolloProvider

abhiaiyer91 reviewed Apr 29, 2016
View reviewed changes

stubailo reviewed May 2, 2016
View reviewed changes

abhiaiyer91 changed the title ~~composeApolloProvider~~ ApolloProvider May 3, 2016

Unify thinking

5d6c806

stubailo reviewed May 3, 2016
View reviewed changes

typescript settings in WS

736a706

stubailo reviewed May 3, 2016
View reviewed changes

Getting that TSLint Grind

69ea1a4

jbaxleyiii merged commit b292fae into apollographql:master May 3, 2016

		@@ -0,0 +1,72 @@
		/// <reference path="../typings/main.d.ts" />
		/* tslint:disable:no-unused-variable */

		@@ -0,0 +1,97 @@
		/// <reference path="../typings/main.d.ts" />

ApolloProvider #36

ApolloProvider #36

Conversation

abhiaiyer91 commented Apr 29, 2016 • edited Loading

abhiaiyer91 commented Apr 29, 2016

abhiaiyer91 commented Apr 29, 2016

stubailo commented Apr 29, 2016

abhiaiyer91 commented Apr 29, 2016

abhiaiyer91 commented Apr 29, 2016

abhiaiyer91 commented Apr 29, 2016

jbaxleyiii commented Apr 29, 2016

stubailo commented Apr 29, 2016

abhiaiyer91 commented Apr 29, 2016

Choose a reason for hiding this comment

jbaxleyiii commented Apr 29, 2016

abhiaiyer91 commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiaiyer91 commented May 2, 2016

stubailo commented May 2, 2016

abhiaiyer91 commented May 3, 2016

stubailo May 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaxleyiii commented May 3, 2016

stubailo commented May 3, 2016

stubailo commented May 3, 2016

jbaxleyiii commented May 3, 2016

abhiaiyer91 commented May 3, 2016

abhiaiyer91 commented May 3, 2016

stubailo commented May 3, 2016

Choose a reason for hiding this comment

abhiaiyer91 commented May 3, 2016

stubailo commented May 3, 2016

stubailo commented May 3, 2016

abhiaiyer91 commented May 3, 2016

abhiaiyer91 commented May 3, 2016

stubailo commented May 3, 2016

abhiaiyer91 commented Apr 29, 2016 •

edited

Loading

stubailo May 3, 2016 •

edited

Loading