Support for `@skip` and `@include` #275

Poincare · 2016-06-10T00:53:33Z

Implements support for @skip and @include (see #237).

TODO:

Update CHANGELOG.md with your change
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

…the same

helfer · 2016-06-13T18:29:14Z

src/queries/directives.ts

+  Field,
+  BooleanValue,
+  Variable,
+} from 'graphql';


Does this import all of graphql now, because the exact path isn't specified?

These are just the types, it doesn't actually import anything afaik

helfer · 2016-06-13T18:52:12Z

Quick question: would it not be possible to apply skip and include directives in a way that's as simple as the way it's done in graphql-js? That way seems pretty straight forward:

https://github.com/graphql/graphql-js/blob/1ed070fed63767e82470ca39d036539b99693c4c/src/execution/execute.js#L437

Poincare · 2016-06-13T19:02:23Z

This implementation is pretty similar other than the fact that it allows us to easily incorporate the addition of other directives once/if the GraphQL spec adds them.

In the GraphQL AST, there is a directives on all Selection instances, meaning that it may be possible to apply directives to Selection instances rather than only Field instances. By introducing the DirectiveResolver type, we would be well-equipped to handle this and it doesn't seem to complicate the @skip and @include implementation too much, I think.

helfer · 2016-06-13T19:25:58Z

The thing that confuses me is that I don't know what it means to resolve a directive? Isn't that just applying a directive?

Also, I'm slightly skeptical that we can easily support other directives. So far all the directives I've seen will require somehow executing the query differently, and even returning the result differently.

Since it's not quite clear what those changes might be, it could make sense to just support skip and include in the simplest way, which is by removing nodes when the directives apply.

I'd be happy to be convinced otherwise, for instance with an example of how the current approach would easily let us support deferred and live directives.

Poincare · 2016-06-13T20:43:49Z

DirectiveResolver seems to be poorly named. It does simply apply the directive.

I was imagining that supporting directives that actually modify the selections within a particular query (e.g. something like @add_typenames or @auth) would be very easy to implement. However, it seems that there was no intention of allowing user-defined queries according to (this)[https://github.com//pull/275] but then a PR was merged that does add custom directives to the schema. I guess it comes down to whether we think directives that modify the selections/selection sets within queries could be useful; if they aren't, I can refactor to just @skip and @include.

stubailo · 2016-06-13T20:49:59Z

For context other directives we might want to support include defer and poll.

Poincare · 2016-06-13T21:46:10Z

After discussion with @helfer, I'll go ahead an implement a shouldInclude function similar to the graphql-js implementation that will be executed inside readFromStore and writeFromStore to determine if a particular field should be expected/included.

Other directives, such as defer, poll, live, etc., will be dealt with separately, likely at the network interface level.

…rbage when it runs

Poincare · 2016-06-16T21:57:02Z

src/store.ts

 } = {}): ApolloStore {
  const enhancers = [];

+  if (reportCrashes === undefined) {


I had to add this because otherwise one of the unit tests would print all of this useless stuff every time it ran because it irked the crash reporter.

I'm not sure I understand, can you show me later?

Use the TypeScript default argument syntax, reportCrashes = true

…ver as part of a query even if they weren't in the store

helfer · 2016-06-17T19:00:55Z

src/data/diffAgainstStore.ts

@@ -120,6 +124,8 @@ export function diffSelectionSetAgainstStore({
  selectionSet.selections.forEach((selection) => {
    // Don't push more than one missing field per field in the query
    let missingFieldPushed = false;
+    const included = shouldInclude(selection, variables);


move this definition down to just before it is used.
and call it something other than included, maybe includeField

helfer · 2016-06-17T19:14:17Z

test/directives.ts

+    assert.deepEqual(query, queryClone);
+  });
+
+  it('throws an error on an unsupported directive', () => {


We don't need this, we can just ignore directives we don't know about.

As discussed, leaving this and the change below in the PR but added a note that mentions that we should move it into the validators at some point.

I'm not so sure about this - all directives I've heard about have required the client to know about them to work - skip, defer, etc.

helfer · 2016-06-17T19:30:57Z

This looks good, you can merge it after some minor changes.

stubailo · 2016-06-21T17:29:29Z

src/data/diffAgainstStore.ts

 }): FieldDiffResult {
  const storeObj = store[rootId] || {};
  const storeFieldKey = storeKeyNameFromField(field, variables);
+  if (included === undefined) {


This should be typeof included === 'undefined'.

We don't need this check at all if we use typescript default arg syntax

stubailo · 2016-06-21T20:37:40Z

test/client.ts

@@ -710,6 +718,88 @@ describe('client', () => {
    });
  });

+  describe('directives', () => {


I think we can move these tests to roundtrip.ts, they will be much more concise there. No need to exercise querymanager for all of them, IMO.

zol added the in progress label Jun 10, 2016

zol assigned Poincare Jun 10, 2016

Poincare added 3 commits June 10, 2016 15:38

added utility functions to add in skip directives and unit tests for …

d89d14d

…the same

added utility functions

948618d

added more tests for include

f53939f

Poincare force-pushed the directives branch from 8dd09d9 to f53939f Compare June 10, 2016 22:38

helfer reviewed Jun 13, 2016
View reviewed changes

Poincare added 4 commits June 16, 2016 10:37

in progress commit for implementing an error on extra fields

5588db1

now throwing errors if the server returns a field that has been skipped

46dab91

added tests that should make coveralls happier

7e1395a

fixed one of the tests so that it now doesn't print out a bunch of ga…

28b9a41

…rbage when it runs

Poincare reviewed Jun 16, 2016
View reviewed changes

Poincare and others added 4 commits June 16, 2016 14:57

fixed an issue where the skipped fields would not get sent to the ser…

ca7518b

…ver as part of a query even if they weren't in the store

fixed issues @stubailo had mentioned

8870462

Merge branch 'master' into directives

f84f45f

updated changelog

20c1ea4

Poincare changed the title ~~First step toward supporting @skip and @include~~ Support for @skip and @include Jun 16, 2016

helfer reviewed Jun 17, 2016
View reviewed changes

Poincare added 4 commits June 17, 2016 13:23

fixed issues with shouldInclude

7cd95d5

fixed style issue

0c1e74c

merged in master

e2b410d

fixed merge conflicts

1cd9f9f

stubailo reviewed Jun 21, 2016
View reviewed changes

Poincare added 3 commits June 21, 2016 11:23

fixes

bced2d4

directives

4bda736

somehow these fixes didn't work last commit

a451fd2

stubailo reviewed Jun 21, 2016
View reviewed changes

Poincare and others added 3 commits June 21, 2016 13:47

moved tests to roundtrip.ts

5df4a87

Merge branch 'master' into directives

132f2e7

Fix changelog

85a7585

stubailo merged commit 8b4ea8a into master Jun 21, 2016

zol removed the in progress label Jun 21, 2016

stubailo deleted the directives branch September 20, 2016 03:41

This was referenced Aug 4, 2017

chore(deps): update dependency ts-jest to version 20.0.8 #1999

Closed

chore(deps): update dependency ts-jest to version 20.0.9 #2000

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `@skip` and `@include` #275

Support for `@skip` and `@include` #275

Poincare commented Jun 10, 2016 •

edited

Loading

helfer Jun 13, 2016

stubailo Jun 13, 2016

helfer commented Jun 13, 2016

Poincare commented Jun 13, 2016

helfer commented Jun 13, 2016

Poincare commented Jun 13, 2016

stubailo commented Jun 13, 2016

Poincare commented Jun 13, 2016

Poincare Jun 16, 2016

helfer Jun 17, 2016

stubailo Jun 21, 2016

helfer Jun 17, 2016 •

edited

Loading

helfer Jun 17, 2016

Poincare Jun 20, 2016

stubailo Jun 21, 2016

helfer commented Jun 17, 2016

stubailo Jun 21, 2016

stubailo Jun 21, 2016

stubailo Jun 21, 2016

Support for @skip and @include #275

Support for @skip and @include #275

Conversation

Poincare commented Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer commented Jun 13, 2016

Poincare commented Jun 13, 2016

helfer commented Jun 13, 2016

Poincare commented Jun 13, 2016

stubailo commented Jun 13, 2016

Poincare commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer Jun 17, 2016 • 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 commented Jun 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support for `@skip` and `@include` #275

Support for `@skip` and `@include` #275

Poincare commented Jun 10, 2016 •

edited

Loading

helfer Jun 17, 2016 •

edited

Loading