Specifically handle the connection directive when selecting store keys, fix #1779 #1801

shadaj · 2017-06-16T21:57:42Z

This gives the client specific knowledge of the @connection directive so that the store key can be properly derived from the directive. This also updates the query transformer to remove the directive before sending requests to the server. This fixes #1779

TODO:

If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change
Add your name and email to the AUTHORS file (optional)

apollo-cla · 2017-06-16T21:57:43Z

@shadaj: 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/

helfer

@shadaj this is a great start! 👏

I've left a few comments in the code for things to change. I think the major ones are around creating an applyTransforms function and pulling together the getStoreKeyName functions into one and splitting out other stuff. Apart from that, it's just about adding more tests to make sure things work as intended.

For example, we should add tests to make sure that fetchMore works as planned with the @connection directive. A good start would be to copy some (or all) of the fetchMore tests and rewrite them to use connections. It should work pretty much as is, but we should still make sure that it actually does.

Another thing we'll have to figure out is whether there's a way to use @connection when we're not using fetchMore. Right now I don't think there is, but if we add special connection types with custom resolvers in the future, I think there will be.

Really great work! 👍

helfer · 2017-06-19T22:08:01Z

src/ApolloClient.ts

@@ -127,6 +127,7 @@ export default class ApolloClient implements DataProxy {
  public queryManager: QueryManager;
  public reducerConfig: ApolloReducerConfig;
  public addTypename: boolean;
+  public removeConnectionDirective: boolean;


I don't think this is necessary, we probably always want to remove the directive.

helfer · 2017-06-19T22:09:20Z

src/core/QueryManager.ts

@@ -135,6 +136,7 @@ export class QueryManager {
  public ssrMode: boolean;

  private addTypename: boolean;
+  private removeConnectionDirective: boolean;


I also don't think we need this.

helfer · 2017-06-19T22:16:31Z

src/data/storeUtils.ts

 }

-export function storeKeyNameFromFieldNameAndArgs(fieldName: string, args?: Object): string {
+export type Directives = {
+    [fieldName: string]: {


Is this supposed to be directiveName?

helfer · 2017-06-19T22:17:47Z

src/data/storeUtils.ts

+};
+
+export function getStoreKeyName(fieldName: string, directives?: Directives, args?: Object): string {
+  if (directives && directives['connection'] && directives['connection']['key']) {


We might want to make debugging easier for people by throwing an error or printing a warning if we encounter a connection directive that doesn't define the key argument.

helfer · 2017-06-19T22:25:02Z

src/core/QueryManager.ts

@@ -1019,6 +1024,10 @@ export class QueryManager {
      queryDoc = addTypenameToDocument(queryDoc);
    }

+    if (this.removeConnectionDirective) {


I'm not sure exactly where transformQueryDoucment gets run from, but we'll have to make sure the directive doesn't get removed before it reaches the cache. It should also still be there when the query is read out of the cache. Only the server should be unaware of it.

helfer · 2017-06-19T22:44:02Z

test/client.ts

+
+    const client = new ApolloClient({
+      networkInterface,
+      addTypename: true,


I think addTypename is true by default, so you don't need to put it here.

helfer · 2017-06-19T22:45:06Z

test/client.ts

+    });
+  });
+
+  it('should not remove the connection directive if there are no arguments', () => {


I think this test shouldn't exist because we should remove the directive anyway (as I've written in the code)

helfer · 2017-06-19T22:47:06Z

test/client.ts

+    return client.query({ query }).then((actualResult) => {
+      assert.deepEqual(actualResult.data, result);
+    });
+  });


There should be an end-to-end test as well that checks that the store contains the right objects after a query. The structure can be the same as for this test.

Add a test to check if it prints a warning when @connection is used without the key argument. You can use withWarning or withError for that.

helfer · 2017-06-19T22:54:32Z

test/readFromStore.ts

@@ -812,4 +812,43 @@ describe('reading from the store', () => {
      nullField: null,
    });
  });
+
+  it('properly handles the connection directive', () => {


I think this is a good test, but what concerns me a little bit now is that we're completely ignoring the arguments on the field and simply return the whole result. I think it's fine right now, but we might want to change it and start supporting different pagination types like skip/offset, cursor etc. which would then use custom resolvers. So the limit connection would look for the skip and limit arguments, and extract only the relevant part from the array. In the example in this test, it would notice that it doesn't have two elements, so it would actually go and fetch those for the server (we may have to add an index to every field in the store so we can start fetching from the middle and don't have to start with offset: 0).

helfer · 2017-06-19T22:56:29Z

test/writeToStore.ts

@@ -1311,4 +1311,62 @@ describe('writing to the store', () => {
      });
    }, /stringField(.|\n)*abcd/g);
  });
+
+  it('properly handles the connection directive', () => {


I think this test is good, but it also shows that whenever we fetch anything that has a connection directive on it, we'll end up overwriting what's already in the store. It may be okay as long as it only works with fetchMore, but I'm pretty sure people will ask for different behaviors, so we may have to add custom write-to-store logic for the connection types. I don't think we'll implement it for the old Redux store, but it's definitely something great to keep in mind for the new store.

helfer · 2017-06-19T23:45:49Z

src/ApolloClient.ts

    this.disableNetworkFetches = ssrMode || ssrForceFetchDelay > 0;
    this.dataId = dataIdFromObject = dataIdFromObject || defaultDataIdFromObject;
    this.dataIdFromObject = this.dataId;
-    this.fieldWithArgs = storeKeyNameFromFieldNameAndArgs;
+    this.fieldWithArgs = (fieldName, args) => getStoreKeyName(fieldName, undefined, args);


Maybe change this to have directives last, that way people can call it with directives.

stubailo · 2017-06-20T19:30:35Z

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### vNEXT

 - `batchInterval` now has a default value of 10 ms [PR #1793](https://github.com/apollographql/apollo-client/pull/1793)
+- the `@connection(key: ...)` directive can now be used to specify the key to use


Should we call it something that doesn't restrict its use to only connections?

helfer

Looks great! I think there are a few more tests to add, but the ones that are here are good 🙂

helfer · 2017-06-21T18:50:34Z

test/client.ts

    });

    return client.query({ query }).then((actualResult) => {
      assert.deepEqual(actualResult.data, result);
    });
  });

-  it('should not remove the connection directive if there are no arguments', () => {
+  it('should run queries that include the connection directive even if there are no arguments', () => {


It's not clear what run means in this context. Does it mean sending them over the network? Does it mean executing them against the store? Or both?

I think this test actually tests that the connection directive gets ignored by the store and removed by the network interface.

helfer · 2017-06-21T18:51:44Z

test/client.ts

@@ -2637,6 +2634,57 @@ describe('client', () => {
  });
 });

+it('should run queries that include the connection directive', () => {


I think this could also be more descriptive. For example something like "runs a query with the connection directive and writes it to the store key defined in the directive" or something like that.

helfer · 2017-06-21T18:55:20Z

test/networkInterface.ts

@@ -483,6 +503,44 @@ describe('network interface', () => {
      });
    });
  });
+
+  describe('transforming queries', () => {
+    it('should remove the @connection directive', () => {


I think this test is good as it is, but another way to test this would be to mock fetch and then check that the right params are passed to it.

The test indirectly checks this, since the existing mocked fetch will only return the expected response if the @connection was removed.

…ery is run

helfer

Unfortunately I think we'll have to solve this at a lower level. updateQuery should get the real current result at the moment the fetchMore query has returned, along with the new fetchMore result. The fetchMore result doesn't need to be written to the store at all, so maybe that will help.

helfer · 2017-06-23T01:34:46Z

test/client.ts

@@ -2634,7 +2634,7 @@ describe('client', () => {
  });
 });

-it('should run queries that include the connection directive', () => {
+it('should run a query with the connection directive and write the result to the store key defined in the directive', () => {


This test description is super similar to the one on line 2558, how are they different?

helfer · 2017-06-23T01:36:42Z

src/core/ObservableQuery.ts

    return Promise.resolve()
      .then(() => {
        const qid = this.queryManager.generateQueryId();
        let combinedOptions: any = null;

+        previous = this.currentResult().data;


I don't think this will actually work, because while the fetchMore query is in flight, the current query's data might change. That change should still be reflected in the previousResult. Instead of bypassing, you'll have to fix the value of previousResult so it is actually the previous result and not the new one. It might be possible to do that by not writing query results to the store if it's a fetchMore query.

helfer · 2017-06-23T01:41:41Z

src/core/ObservableQuery.ts

@@ -229,15 +229,13 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
      throw new Error('updateQuery option is required. This function defines how to update the query data with the new results.');
    }

-    let previous: any;
+    const previous = this.result();


I think this has the same semantics wrt when it runs, which means it still doesn't fix the race condition of an update while the query is in flight.

helfer · 2017-06-24T00:47:06Z

Merged! 🎉
Nice work @shadaj 🙂

scf4 · 2017-07-09T20:11:37Z

I'm all up-to-date but getting unknown directive when trying to use this. Am I missing something?

query thread($id: Int! $offset: Int $limit: Int){
  thread(id: $id) {
    ...
    posts(offset: $offset limit: $limit) @connection(key: "posts") {
      ...
    }
  }
}

stubailo · 2017-07-09T21:04:12Z

Can you open a new issue about this please @scf4? Also, can you post the full error with stack trace and some code as well?

juhaelee · 2017-07-25T04:28:46Z

@stubailo im also getting an unknown directive, but only when im using persisted queries. here's my stack trace:

{ Error: GraphQL error: Unknown directive "connection".
    at new ApolloError (/Users/Projects/client/node_modules/apollo-client/src/errors/ApolloError.js:34:28)
    at /Users/Projects/client/node_modules/apollo-client/src/core/QueryManager.js:322:41
    at /Users/Projects/client/node_modules/apollo-client/src/core/QueryManager.js:816:25
    at Array.forEach (native)
    at /Users/Projects/client/node_modules/apollo-client/src/core/QueryManager.js:813:27
    at Array.forEach (native)
    at QueryManager.broadcastQueries (/Users/Projects/client/node_modules/apollo-client/src/core/QueryManager.js:810:42)
    at /Users/Projects/client/node_modules/apollo-client/src/core/QueryManager.js:745:31
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:169:7)
  graphQLErrors:
   [ { message: 'Unknown directive "connection".',
       locations: [Array] } ],
  networkError: null,
  message: 'GraphQL error: Unknown directive "connection".',
  extraInfo: undefined,
  queryErrors: [ [Circular] ] }

stubailo · 2017-07-25T04:33:20Z

Oh, that's interesting! Looks like the persisted queries tool hasn't been updated to remove the @connection directive before the query is sent to the server. Can you file an issue there?

shadaj force-pushed the connection-directive-handling branch 2 times, most recently from e06e62b to 312ffaa Compare June 16, 2017 22:10

helfer reviewed Jun 19, 2017

View reviewed changes

stubailo reviewed Jun 20, 2017

View reviewed changes

shadaj force-pushed the connection-directive-handling branch 2 times, most recently from af496bb to 64c3262 Compare June 21, 2017 17:22

helfer approved these changes Jun 21, 2017

View reviewed changes

shadaj added 6 commits June 22, 2017 15:59

Specifically handle the connection directive when selecting store keys

f71304f

Update authors and changelog

a6f7af4

Shift query transformation to network interface

dd32cb6

Improve unit test naming

c8ec6fb

Modify fetchMore logic to store the previous result before the new qu…

699ddd6

…ery is run

Make unit tests for fetchMore pass

6d62776

shadaj force-pushed the connection-directive-handling branch from 669ef44 to 6d62776 Compare June 22, 2017 22:59

helfer suggested changes Jun 23, 2017

View reviewed changes

shadaj force-pushed the connection-directive-handling branch from 5d54f8d to c1c6552 Compare June 23, 2017 22:04

Do not write fetchMore results to store

3e136bd

shadaj force-pushed the connection-directive-handling branch from c1c6552 to 3e136bd Compare June 23, 2017 22:05

helfer added 2 commits June 23, 2017 17:33

Add comment on fetchMore branch logic

33aaf02

Merge branch 'master' into connection-directive-handling

4f4c455

helfer approved these changes Jun 24, 2017

View reviewed changes

helfer merged commit 94d8d20 into master Jun 24, 2017

helfer deleted the connection-directive-handling branch June 24, 2017 00:47

lxwbr mentioned this pull request Jun 29, 2017

Adjusting the store in order to squash pagination results via Relay's @connection directive. #1779

Closed

jonmanzo mentioned this pull request Jul 7, 2017

Updating cached data from multiple parameterized queries after a mutation apollographql/react-apollo#708

Closed

juhaelee mentioned this pull request Jul 25, 2017

Remove the @connection directive before the query is sent to the server apollographql/persistgraphql#37

Open

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specifically handle the connection directive when selecting store keys, fix #1779 #1801

Specifically handle the connection directive when selecting store keys, fix #1779 #1801

shadaj commented Jun 16, 2017 •

edited

Loading

apollo-cla commented Jun 16, 2017

helfer left a comment

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017

helfer Jun 19, 2017 •

edited

Loading

helfer Jun 19, 2017 •

edited

Loading

helfer Jun 19, 2017

helfer Jun 19, 2017

stubailo Jun 20, 2017

helfer left a comment

helfer Jun 21, 2017

helfer Jun 21, 2017

helfer Jun 21, 2017

helfer Jun 21, 2017

shadaj Jun 21, 2017

helfer left a comment

helfer Jun 23, 2017

helfer Jun 23, 2017

helfer Jun 23, 2017

helfer commented Jun 24, 2017

scf4 commented Jul 9, 2017 •

edited

Loading

stubailo commented Jul 9, 2017

juhaelee commented Jul 25, 2017

stubailo commented Jul 25, 2017

Specifically handle the connection directive when selecting store keys, fix #1779 #1801

Specifically handle the connection directive when selecting store keys, fix #1779 #1801

Conversation

shadaj commented Jun 16, 2017 • edited Loading

apollo-cla commented Jun 16, 2017

helfer left a 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

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

helfer Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

helfer Jun 19, 2017 • edited Loading

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

helfer left a 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

Choose a reason for hiding this comment

helfer left a 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

helfer commented Jun 24, 2017

scf4 commented Jul 9, 2017 • edited Loading

stubailo commented Jul 9, 2017

juhaelee commented Jul 25, 2017

stubailo commented Jul 25, 2017

shadaj commented Jun 16, 2017 •

edited

Loading

helfer Jun 19, 2017 •

edited

Loading

helfer Jun 19, 2017 •

edited

Loading

scf4 commented Jul 9, 2017 •

edited

Loading