Add UpdateQuery for FetchMore #99

jeffutter · 2018-05-31T15:39:09Z

updateQuery for fetchMore

Pull Request Labels

feature
blocking
docs

fixes #79

This sort of works but the solution doesn't seem ideal. I'm posting it now to get some feedback. Does this seem like a reasonable approach?

Also, I plan to squash the PR once the solution seems agreed upon, but I would like to keep some of my past implementations for the time being until a solution is settled upon.

Gregoirevda · 2018-05-31T20:32:24Z

src/graphql-types/ReasonApolloQuery.re

      }),
    networkStatus: apolloData##networkStatus,
  };
  let make =
      (
        ~variables: option(Js.Json.t)=?,
+        ~updateQuery: option((Config.t, updateQueryOptions) => Config.t)=?,


This is tricky, because Config.t is the initial Query, but you should be able to pass any query in the updateQuery?

I'm not sure if that is correct. My understanding is that updateQuery isn't the best name, it's not a new query it's just a function to combine the results from the initial query and a fetchMore query. The fetchMore query is the original query but can have new variables.

I'm getting this information from: https://www.apollographql.com/docs/react/features/pagination.html I may be interpreting it incorrectly.

In the link there is a mention:

note this is a different query than the one used in the Query component

updateQuery shouldn't be a prop of the Query component. It's only a property of the config o ject passed to fetchMore

jeffutter · 2018-06-01T02:59:40Z

updateQuery shouldn't be a prop of the Query component. It's only a property of the config o ject passed to fetchMore

Good point, I've updated the PR to reflect that.

note this is a different query than the one used in the Query component

Oh yeah, I just noticed that fetchMore takes an optional query parameter. That poses an interesting issue for typing that I'm not sure how to address. For now I'll continue ahead typing it the same as the original query and circle back once it is working.

It is closer to working now. The query executes but my combine function has some issue. Something is coming through as non-optional that I'm expecting to be optional. I'll keep plugging away.

jeffutter · 2018-06-01T17:17:16Z

I'm pretty much stuck here. I can't get the typing to work out:

https://github.com/jeffutter/reason-apollo/blob/e4dfefdea61974e2bf78468e879e0dee7c56b12a/src/graphql-types/ReasonApolloQuery.re#L34-L39

The updateQuery function doesn't actually take Config.t's The Config.t types have stripped out the __type fields from the original payload and updateQuery is expected to return the raw objects that include the __type fields. I've tried losening the types all the way to just calling them Js.t({.}) but that isn't working either.

Any ideas?

jeffutter · 2018-06-04T02:52:00Z

With the last commit, this "sort of" works. The function I pass in to fetchMore from my code gets: (Js.Json.t, {"fetchMoreResults": Js.Json.t}) I then have to do something like:

external unsafeFromJson : Js.Json.t => responseJS = "%identity";
external unsafeToJson : responseJS => Js.Json.t = "%identity";

...

let updateQuery = (previousResult, newResults) => {
  let previousResult = unsafeFromJson(previousResult);
  (
    switch (newResults##fetchMoreResult) {
    | Some(fetchMoreResult) =>
      let fetchMoreResult = unsafeFromJson(fetchMoreResult);
...

Where responseJS is a fleshed out version of what would be returned in Javascript.

It's not quite ideal but it works. Any suggestions on a better way would be appreciated.

ulrikstrid · 2018-06-04T11:20:08Z

src/ReasonApolloTypes.re

@@ -8,7 +8,7 @@ type queryString;
 * query string to the standard GraphQL AST.


As there are no actual changes in this file, just refmt, it should probably not be in this PR.

ulrikstrid · 2018-06-05T10:08:30Z

src/graphql-types/ReasonApolloQuery.re

@@ -8,6 +8,23 @@ module Get = (Config: ReasonApolloTypes.Config) => {
    | Loading
    | Error(apolloError)
    | Data(Config.t);
+  type updateQueryOptions = {


If you change this to

[@bs.deriving abstract] type updateQueryOptions = { fetchMoreResult: option(Js.Json.t), variables: Js.Json.t, };

you can remove the next type declaration as they are then the same.

And you can simplify the switch at line 122 to:

switch (updateQuery) { | Some(updateQuery) => Some( ( (previousResult, opts) => updateQuery(previousResult, opts) ), ) | None => None }

That seems to have a small side effect. In the application code that is using this: opts##fetchMoreResult is no longer optional. It seems like it should be based on: https://www.apollographql.com/docs/react/features/pagination.html#numbered-pages shouldn't it?

jeffutter · 2018-06-05T20:24:27Z

I think I've gotten the types sorted out here with the help of @ulrikstrid . Anyone else have any comments on this PR?

jeffutter · 2018-06-07T11:46:10Z

Any thoughts on getting this merged?

emmenko

LGTM 👌

Gregoirevda · 2018-06-11T20:18:35Z

I added a commit containing:

An example
updateQueryOptions: variables are also optional (https://github.com/apollographql/apollo-client/blob/master/packages/apollo-client/src/core/ObservableQuery.ts#L44)
type updateQuery = (Js.Json.t, updateQueryOptions) => Js.Json.t; -> type updateQueryT = (Config.t, updateQueryOptions) => Js.Json.t; (not stripping out __typename)
fetchMoreOptions: object containing variables, query and updateQuery. updateQuery is the only required argument. I deleted query instead of assigning the same queryAST (will do that by default)
fetchMore type/function needs a unit at the end.

If you see any error, please comment!
Regarding type updateQueryT = (Config.t, updateQueryOptions) => Js.Json.t; :
It means you wont be able to access __typename of the first argument, but it makes it much easier to manipulate the data.
The return value has to be a Js.Json.t otherwise __typename isn't accepted, which unfortunately makes the return verbose!

I'm going to open a request in graphql_ppx, because apollo-client recommends adding __typename to Typescript and Flow. This will make it much easier to manipulate the previous and next GraphQL results

jeffutter · 2018-06-11T21:00:06Z

@Gregoirevda I'm trying to update my application code to support these changes. I'm not sure about the use of

type updateQueryT = (Config.t, updateQueryOptions) => Js.Json.t;

The first argument isn't a Config.t it's a Js.Json.t. Using a Config.t the compiler makes me handle a bunch of optional values, however, what is actually getting passed in has no optionals, just JS values that may be null. As a result, I get index.js:2178 TypeError: Cannot read property 'whatever' of undefined because it unwraps an optional that isn't an optional and then tries to call a value on it.

jeffutter · 2018-06-13T17:35:57Z

Any update on this?

Gregoirevda · 2018-06-25T20:04:10Z

src/graphql-types/ReasonApolloQuery.re

+  [@bs.deriving abstract]
+  type updateQueryOptions = {
+    [@bs.optional]
+    fetchMoreResult: Config.t,


What about fetchMoreResult? It should be a Js.Json.t no?

Yes, I believe so.

Gregoirevda · 2018-06-25T20:06:33Z

I asked for a feature in graphql_ppx for __typename fields support. This is related to this issue.
mhallin/graphql_ppx#39 (comment)

Gregoirevda · 2018-07-08T22:02:49Z

Sorry for taking so long. I made the Config.t a Js.Json.t
Took the same logic in subscribeToMore in the Query component

jeffutter · 2018-07-09T11:30:37Z

Just tested this out on master, works great. Thanks!

Gregoirevda · 2018-07-09T12:10:33Z

I could just not post a new big example project yesterday evening, will push asap

Enalmada · 2018-12-04T23:02:40Z

Can someone post how to make the reason-apollo swapi example actually update Apollo Store? Right now there is just a placeholder for it:

~updateQuery= (prev, _next) => {
   /* Update Apollo Store with [@bs.raw {||}] for now, since the type coming in is a generic Js.Json.t for now*/
   prev;
}

I see here in the apollo docs that the js version looks like this:

updateQuery: (prev, { fetchMoreResult }) => {
              if (!fetchMoreResult) return prev;
              return Object.assign({}, prev, {
                feed: [...prev.allPersons, ...fetchMoreResult.allPersons]
              });
            }

This kinda works but it is not handling the if (!fetchMoreResult) return prev; case. Could/should this be done better?

~updateQuery= (prev, next) => {   
    [%bs.raw {|
       Object.assign({}, prev, {allPersons: [...prev.allPersons, ...next.fetchMoreResult.allPersons]})
     |}]
}

ghost added the blocking Prevents production or dev due to perf, bug, build error, etc.. label May 31, 2018

jeffutter mentioned this pull request May 31, 2018

updateQuery for fetchMore #79

Closed

5 tasks

Gregoirevda reviewed May 31, 2018

View reviewed changes

ulrikstrid reviewed Jun 4, 2018

View reviewed changes

ulrikstrid reviewed Jun 5, 2018

View reviewed changes

jeffutter force-pushed the updateQuery branch from 6e8c850 to c2f3bd9 Compare June 5, 2018 20:22

jeffutter and others added 3 commits June 5, 2018 15:22

Add UpdateQuery for FetchMore

b96ce84

Simplify with bs.deriving

126cc45

Final cleanup with bs.deriving for optional fields

75db3b4

jeffutter force-pushed the updateQuery branch from c2f3bd9 to 75db3b4 Compare June 5, 2018 20:23

emmenko reviewed Jun 7, 2018

View reviewed changes

Fix minors && add example

098e1e9

Gregoirevda reviewed Jun 25, 2018

View reviewed changes

Gregoirevda added 4 commits July 8, 2018 23:30

Fix Config.t -> Js.Json.t

ae44012

Fix example accordingly

556233a

Merge branch 'master' into updateQuery

b405c6b

resolve conflicts

4514f32

Gregoirevda merged commit 7eef7dd into apollographql:master Jul 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UpdateQuery for FetchMore #99

Add UpdateQuery for FetchMore #99

jeffutter commented May 31, 2018 •

edited

Gregoirevda May 31, 2018

jeffutter May 31, 2018

Gregoirevda May 31, 2018

Gregoirevda May 31, 2018

jeffutter commented Jun 1, 2018

jeffutter commented Jun 1, 2018

jeffutter commented Jun 4, 2018

ulrikstrid Jun 4, 2018

ulrikstrid Jun 5, 2018

jeffutter Jun 5, 2018

jeffutter commented Jun 5, 2018

jeffutter commented Jun 7, 2018

emmenko left a comment

Gregoirevda commented Jun 11, 2018

jeffutter commented Jun 11, 2018

jeffutter commented Jun 13, 2018

Gregoirevda Jun 25, 2018

jeffutter Jun 25, 2018

Gregoirevda commented Jun 25, 2018

Gregoirevda commented Jul 8, 2018

jeffutter commented Jul 9, 2018

Gregoirevda commented Jul 9, 2018

Enalmada commented Dec 4, 2018

		@@ -8,7 +8,7 @@ type queryString;
		* query string to the standard GraphQL AST.

Add UpdateQuery for FetchMore #99

Add UpdateQuery for FetchMore #99

Conversation

jeffutter commented May 31, 2018 • edited

updateQuery for fetchMore

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

jeffutter commented Jun 1, 2018

jeffutter commented Jun 1, 2018

jeffutter commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffutter commented Jun 5, 2018

jeffutter commented Jun 7, 2018

emmenko left a comment

Choose a reason for hiding this comment

Gregoirevda commented Jun 11, 2018

jeffutter commented Jun 11, 2018

jeffutter commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gregoirevda commented Jun 25, 2018

Gregoirevda commented Jul 8, 2018

jeffutter commented Jul 9, 2018

Gregoirevda commented Jul 9, 2018

Enalmada commented Dec 4, 2018

jeffutter commented May 31, 2018 •

edited