Fix to support both server and local-state access in same query #193

akiran · 2018-02-02T12:05:21Z

In this PR, I am trying to fix the errors thrown when both server and local-state data is accessed in same query

Error is happening because of this line. resolverMap is undefined for query. so error is thrown when resolverMap[fieldName] is accessed. I fixed this by adding {} as fallback to resolverMap

Also rootValue passed to resolver has only server data.
if resolvers are not explicitly defined, apollo-link-state could not return data for client query.
To fix this, I wrote a utility getDirectivesFromDocument to get a query with client only fields and read data from cache and merged with server data to pass it to resolver.
https://github.com/akiran/apollo-link-state/blob/combine-server-client-query/packages/apollo-link-state/src/index.ts#L85

This POC is working and I was able to get both server and client data with same query.
@peggyrayzis Can you review this and let me know if the direction of this solution is correct.

If the direction is correct, I will handle all the edge cases and write tests.

brunobasto · 2018-02-02T12:25:24Z

Awesome!!

akiran · 2018-02-02T13:02:29Z

This PR also fixes #187

fbartho · 2018-02-02T20:33:38Z

This looks really cool, I look forward to using this when it's done @akiran. Having some tests would certainly help make sure this functionality isn't forgotten in the future!

akiran · 2018-02-03T04:00:48Z

@fbartho Thanks, I will complete this PR and add tests shortly

codecov-io · 2018-02-06T07:45:44Z

Codecov Report

Merging #193 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #193   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files           2        2           
  Lines          62       62           
  Branches       19       19           
=======================================
  Hits           61       61           
  Partials        1        1

Impacted Files	Coverage Δ
packages/apollo-link-state/src/utils.ts	`100% <ø> (ø)`	⬆️
packages/apollo-link-state/src/index.ts	`98.03% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d252fcf...d3ad47f. Read the comment docs.

peggyrayzis

Hi @akiran, thanks for taking this on! 😀 Can you please move getDirectivesFromDocument to a separate PR on the apollo-client repo for apollo-utilities? Then, we can update this PR once it's released.

peggyrayzis · 2018-02-07T20:31:56Z

Here's the associated issue for apollo-utilities: apollographql/apollo-client#2884

akiran · 2018-02-08T09:23:55Z

@peggyrayzis I created PR for getDirectivesFromDocument apollographql/apollo-client#2974

akiran · 2018-02-08T09:26:01Z

@peggyrayzis I need a merge function in this PR to support nested queries that has both client and server fields. Should I create a new utility in apollo-utilities or can we use lodash.merge ?

akiran · 2018-02-12T11:14:29Z

@peggyrayzis @fbartho Can you review this

majelbstoat · 2018-02-13T07:58:21Z

packages/apollo-link-state/package.json

@@ -79,7 +79,8 @@
  },
  "dependencies": {
    "apollo-utilities": "^1.0.6",
-    "graphql-anywhere": "^4.1.0-alpha.0"
+    "graphql-anywhere": "^4.1.0-alpha.0",
+    "lodash": "^4.17.5"


If merge is the only thing you're using, you could instead depend on 'lodash.merge'.

@majelbstoat I tried lodash.merge. ES2015 style import didn't work with it. It is only working only if I use require statement

Lodash & the TS compiler are weird sometimes 😢

Try import merge = require('lodash/merge'); for the submodule and npm i @types/lodash -D so we have the proper typings. This is how we do it in react-apollo.

peggyrayzis

@akiran Before I give this a final review, can you please fix the merge conflict & confirm all tests are still passing, upgrade apollo-utilities, and remove all code/tests related to getDirectivesFromDocument? We just released your getDirectivesFromDocument changes in the latest release of apollo-utilities. 🎉 Thanks!

Ideally, I'd like to get this merged today.

akiran · 2018-02-13T16:01:18Z

@peggyrayzis Latest version of apollo-utilities on npm is still 1.0.6.
Can you confirm new version of apollo is released

peggyrayzis · 2018-02-13T16:20:15Z

Oops @akiran, looks like it didn't publish due to the npm outages last week. I'll let you know when it's ready!

akiran · 2018-02-13T16:27:50Z

@peggyrayzis Can you hold off on apollo-utilities release for a while.
After resolving merge conflicts, some recently added tests are failing in this repo.
This could be an issue with getDirectivesFromDocument. I think, I didn't handled cases where query is combined with mutations.

I will test it and confirm you shortly

akiran · 2018-02-13T16:46:59Z

@peggyrayzis We needn't support this type of query where query and mutation are combined right ?

const query = gql`
      query SampleQuery {
        user @client
        product
      }
      mutation SampleMutation {
        login @client
        updateUser
      }
    `;

When I run a test with this query on getDirectivesFromDocument, I got below error
Ambiguous GraphQL document: contains 2 operations

akiran · 2018-02-13T16:55:05Z

@peggyrayzis I think, there is no issue with getDirectivesFromDocument
I wrote additional tests to make sure it works with mutations as well
apollographql/apollo-client#2995
You can release apollo-utilities

I will debug why tests are failing in this repo and fix it

peggyrayzis · 2018-02-13T17:02:11Z

@peggyrayzis We needn't support this type of query where query and mutation are combined right ?

No, I don't think so. Thanks for writing the tests to confirm it works with mutations!

@evans do you have any idea why the tests would be failing here with the new resetStore changes?

akiran · 2018-02-13T17:30:07Z

packages/apollo-link-state/src/index.ts

@@ -83,6 +87,8 @@ export const withClientState = (
      };

      return new Observable(observer => {
+        const clientQuery = getClientSetsFromDocument(operation.query);
+        const clientData = cache && cache.readQuery({ query: clientQuery });


@evans @peggyrayzis readQuery in line 91 is causing the tests to fails.

@akiran Thank you so much for looking into this! This client query should be done in the graphql call, try changing the query argument here to clientQuery

The reason for the failure is due to the cache being empty on the first read. The client-side data is populated by the graphql call. If you could add a comment above that call saying that this is where the client data is fetched, that would be incredible!

akiran · 2018-02-13T17:45:16Z

@evans @peggyrayzis I was trying to understand how resetStore and writeDefaults should work for apollo-link-state

When resetStore is triggered, first stored is reseted and then active queries are executed. only after that callback attached to onResetStore will be executed

But for apollo-link-state, shouldn't we execute onResetStore callback (writeDefaults) before executing active queries. otherwise rootValue will be undefined and all the active query execution will fail

peggyrayzis · 2018-02-13T18:39:11Z

But for apollo-link-state, shouldn't we execute onResetStore callback (writeDefaults) before executing active queries. otherwise rootValue will be undefined and all the active query execution will fail

Yeah, that was originally what I wanted to do. client.resetStore() is tricky because the function both empties the store AND refetches all active queries. I tried splitting up the function here to allow for the callbacks in between but wasn't able to get the tests to pass. Maybe a second pair of eyes would help?

In the meantime, @evans recently made a PR to check for the presence of a resolverMap that didn't require making changes to the client core.

akiran · 2018-02-14T17:03:58Z

@peggyrayzis I was able to fix 2 tests in this your PR
apollographql/apollo-client#3010

1 more test is still failing

akiran · 2018-02-16T17:13:21Z

@peggyrayzis @evans Now the tests are passing
I also upgraded to latest version of apollo-utilities and removed getDirectivesFromDocument logic from this repo

akiran · 2018-02-19T10:30:17Z

@evans It is possible to fallback to cache instead of defaults here ?
https://github.com/apollographql/apollo-link-state/blob/master/packages/apollo-link-state/src/index.ts#L82

I didn't find a easy way to do that. So I was reading data from cache with readQuery https://github.com/akiran/apollo-link-state/blob/e02d692cf53cc09ee728877a78ecfb225538cc04/packages/apollo-link-state/src/index.ts#L99
Because of this #L82 will not execute. It either gets data from cache or execute resolver

evans · 2018-02-26T23:54:10Z

@akiran It looks like getDirectivesFromDocument doesn't properly respect fragments included in the query. There are probably bugs with the current solution that we are thinking about. Here is the minimal test case:

const query = gql`
    fragment client on ClientData {
      bar
    }

    query Mixed {
      foo @client {
        ...client
      }
      bar {
        baz
      }
    }
  `;

Returns:

      fragment client on ClientData {}

      query Mixed {
        foo @client {
          ...client
        }
      }

evans · 2018-02-27T00:08:07Z

Here is my proposed patch: clientQuery.patch.txt. Add it with a git apply clientQuery.patch.txt.

The test that uses a fragment fails and we'll need to make the change to AC first. I think we can knock out this PR in the same swing

…move it apollo-utilities later

…former

…t logic from this repo

evans · 2018-03-06T23:46:13Z

@akiran Thank you so much for your investigation, tests, and implementation! I think we've settled on the issue. One of the tests was incorrectly stubbing server behavior. If you get a chance to take a look at this PR, please do.

@jbaxleyiii I'm unsure why this test is passing, since apollo-link-state does not pass back the correct data including the client-side data(indicated by a warning) However the watch query is indeed getting the correct values. I think we need to ensure that the correct data is passed back to through the link chain, so we'll need to add a read from cache in our graphql-anywhere resolver.

Still on our list before 1.0 is figuring out how to deal with the setting of defaults in the onResetStore callback and storing an empty array in the store as a default.

wKovacs64 · 2018-03-09T16:19:12Z

Do you know if this PR is blocking a release that includes your #202, @evans?

evans · 2018-03-09T18:58:07Z

@akiran Thank you so much for the tests and research!

@wKovacs64 We're working on permissions now

akiran · 2018-03-10T07:13:51Z

Thanks @evans

pvlv · 2018-03-13T14:51:18Z

@peggyrayzis when update the package in npm?

peggyrayzis · 2018-03-13T15:08:17Z

I have to cut a release of AC first. I should have it out within the next hour or so.

akiran mentioned this pull request Feb 2, 2018

apollo client resetStore triggers an error related to the defaults from apollo-link-state #194

Closed

akiran force-pushed the combine-server-client-query branch from 5b95551 to a372589 Compare February 6, 2018 07:45

peggyrayzis suggested changes Feb 7, 2018

View reviewed changes

akiran mentioned this pull request Feb 8, 2018

Get directives from document apollographql/apollo-client#2974

Merged

akiran changed the title ~~[Wip] Fix to support both server and local-state access in same query~~ Fix to support both server and local-state access in same query Feb 12, 2018

akiran mentioned this pull request Feb 13, 2018

Mixing client and server state in query leads to error #140

Closed

majelbstoat reviewed Feb 13, 2018

View reviewed changes

peggyrayzis suggested changes Feb 13, 2018

View reviewed changes

akiran commented Feb 13, 2018

View reviewed changes

akiran and others added 19 commits March 6, 2018 15:21

Fix to support both server and local-state access in same query

88d8f60

fix to make existing tests pass

d0d4ab8

moved getDirectivesFromDocument to transform file to make it easy to …

ff3ac72

…move it apollo-utilities later

added tests for getDirectivesFromDocument transform

1691740

Added support for fragment parsing in getDirectivesFromDocument trans…

61b1e4d

…former

fix type issues

73f5b06

using merge instead of assign to combine server and client data

feb5639

fix merge import

3ac9161

added a simple test for querying client and server fields in same query

9847f00

added test for nested query for client and server data

c4e8306

Fix test

b15a81a

Updated apollo-utilities version and removed getDirectivesFromDocumen…

48c804d

…t logic from this repo

invoke read query only for query operations

6a785eb

Test for client and server mutations in same query

d07644d

add react-scripts as devDependency for tests

b583558

wrap expects in try with done fail and fix alias test

47b3dbf

ignore lodash in the bundlesize calculation

9f5a2c0

remove lodash and collect server data from root value

af686c0

remove dependency on lodash

03e4575

evans force-pushed the combine-server-client-query branch from 125e99e to 03e4575 Compare March 6, 2018 23:23

Update utils.ts

d3ad47f

evans merged commit b81edfd into apollographql:master Mar 9, 2018

lorensr mentioned this pull request Aug 29, 2018

Adding local data to a remote query makes remote data undefined #290

Open

Fix to support both server and local-state access in same query #193

Fix to support both server and local-state access in same query #193

Conversation

akiran commented Feb 2, 2018 • edited Loading

brunobasto commented Feb 2, 2018

akiran commented Feb 2, 2018

fbartho commented Feb 2, 2018

akiran commented Feb 3, 2018

codecov-io commented Feb 6, 2018 • edited Loading

Codecov Report

peggyrayzis left a comment

Choose a reason for hiding this comment

peggyrayzis commented Feb 7, 2018

akiran commented Feb 8, 2018

akiran commented Feb 8, 2018

akiran commented Feb 12, 2018

majelbstoat Feb 13, 2018

Choose a reason for hiding this comment

akiran Feb 13, 2018

Choose a reason for hiding this comment

peggyrayzis Feb 13, 2018

Choose a reason for hiding this comment

peggyrayzis left a comment

Choose a reason for hiding this comment

akiran commented Feb 13, 2018

peggyrayzis commented Feb 13, 2018

akiran commented Feb 13, 2018 • edited Loading

akiran commented Feb 13, 2018 • edited Loading

akiran commented Feb 13, 2018

peggyrayzis commented Feb 13, 2018

akiran Feb 13, 2018

Choose a reason for hiding this comment

evans Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

evans Feb 14, 2018

Choose a reason for hiding this comment

akiran commented Feb 13, 2018

peggyrayzis commented Feb 13, 2018

akiran commented Feb 14, 2018

akiran commented Feb 16, 2018

akiran commented Feb 19, 2018

evans commented Feb 26, 2018

evans commented Feb 27, 2018

evans commented Mar 6, 2018

wKovacs64 commented Mar 9, 2018

evans commented Mar 9, 2018

akiran commented Mar 10, 2018

pvlv commented Mar 13, 2018

peggyrayzis commented Mar 13, 2018

akiran commented Feb 2, 2018 •

edited

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

akiran commented Feb 13, 2018 •

edited

Loading

akiran commented Feb 13, 2018 •

edited

Loading

evans Feb 14, 2018 •

edited

Loading