CF-897: Fix directives #267

BrandonHermanAvant · 2020-03-11T20:05:30Z

This updates fixes directives in the current v2 branch.

BrandonHermanAvant · 2020-03-11T20:06:42Z

__tests__/__snapshots__/from-query-normalSchema-test.ts.snap

+exports[`directives fragments works with inline fragments on base 1`] = `
+"export interface FragmentSelectionOnHuman {}
+
+export interface FragmentSelectionOnDroid {
  primaryFunction: string | null;
  primaryFunctionNonNull: string;
 }
-",
-    ],
-    "interface": "export interface FragmentTest {
-  heroNoParam: Partial<IFragmentSpreadOnDroid> | null;
+
+export interface FragmentTest {
+  heroNoParam?: FragmentSelectionOnHuman | FragmentSelectionOnDroid | null;
 }


This is the one new test I added

BrandonHermanAvant · 2020-03-11T20:06:53Z

__tests__/from-query-normalSchema-test.ts

+    it('works with inline fragments on base', () => {
+      const response: string = runProgram(
+        schema,
+        inlineFragmentWithDirectiveOnBaseQuery,
+        undefined,
+        { generateSubTypeInterfaceName }
+      );
+      expect(response).toMatchSnapshot();
+    });


brettjurgens

unsolicited feedback 😅

brettjurgens · 2020-03-11T21:47:30Z

packages/from-query/src/generate.ts

@@ -110,8 +123,15 @@ export default (OPTIONS: IFromQueryOptions): (ir: IOperation) => string => {
    private buildDeclaration (selection: Selection): string {
      switch (selection.kind) {
        case 'Field':
+          const foo = !!selection.directives['include'] || !!selection.directives['skip']


can we find a better name than foo 😄

can we reuse the hasDirectives above? we can then do it inline below

{ // ... optional: hasDirectives(selection.directives) }

ooh good catch, got excited that I got it working and missed this cleanup spot lol

brettjurgens · 2020-03-11T21:48:34Z

packages/from-query/__tests__/from-query-test.ts

+  //       }
+  //     }`)
+  //   ).toMatchSnapshot();
+  // });


why are these tests commented out?

They were failing before our changes. Maybe I should leave them in since this is just going into v2.0.0-3 branch?

The error is:

InlineFragment Must Be Inlined!

brettjurgens · 2020-03-11T21:49:05Z

__tests__/from-query-normalSchema-test.ts

-    );
-    expect(response).toMatchSnapshot();
-  });
+  // it('supports unions', () => {


do unions no longer work? why are these commented out?

Currently getting the following error:

TypeError: Cannot read property 'type' of null

brettjurgens · 2020-03-11T22:32:06Z

packages/from-query/src/generate.ts

-          );
+
+          const selectionHasDirectives = hasDirectives(selection.directives);
+          const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives))


An interesting thing to bring up is the defer directive. what should happen there? Should it transform a field from T -> T | null? Should it generate two types one for the deferred and one for the final result and then OR them?

What about custom directives? This was something I gave a little thought to but didn't come up with a consensus

I think re: custom directives it was an idea to allow the user to provide a "transform" function of sorts. but I don't exactly know what that looks like

brettjurgens · 2020-03-11T22:32:58Z

packages/from-query/src/generate.ts

-          );
+
+          const selectionHasDirectives = hasDirectives(selection.directives);
+          const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives))


Suggested change

const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives))

const fragmentsHaveDirectives = selection.fragments.some(frag => hasDirectives(frag.directives))

!! is unnecessary here as Array.prototype.some will always return a bool

brettjurgens · 2020-03-11T22:36:42Z

packages/from-query/src/generate.ts

@@ -110,8 +123,15 @@ export default (OPTIONS: IFromQueryOptions): (ir: IOperation) => string => {
    private buildDeclaration (selection: Selection): string {
      switch (selection.kind) {
        case 'Field':
+          const foo = !!selection.directives['include'] || !!selection.directives['skip']


can we reuse the hasDirectives above? we can then do it inline below

{ // ... optional: hasDirectives(selection.directives) }

brettjurgens · 2020-03-11T22:38:34Z

packages/from-query/src/generate.ts

@@ -66,7 +66,20 @@ export default (OPTIONS: IFromQueryOptions): (ir: IOperation) => string => {
    }
  };

-  const printField: (name: string, type: TypeDefinition | string, node: Selection, nameOverride?: string) => string = (name, type, node, nameOverride) => `${name}: ${printType(type, node, nameOverride)};`;
+  const hasDirectives: (directives: IDirectiveMap) => boolean = directives =>
+    !!directives['include'] || !!directives['skip']


Suggested change

!!directives['include'] || !!directives['skip']

!!directives.include || !!directives.skip

you can use dot-notation

brettjurgens · 2020-03-11T22:40:09Z

packages/from-query/src/generate.ts

@@ -66,7 +66,20 @@ export default (OPTIONS: IFromQueryOptions): (ir: IOperation) => string => {
    }
  };

-  const printField: (name: string, type: TypeDefinition | string, node: Selection, nameOverride?: string) => string = (name, type, node, nameOverride) => `${name}: ${printType(type, node, nameOverride)};`;
+  const hasDirectives: (directives: IDirectiveMap) => boolean = directives =>


can we rename this to convey that it checks for specific directives instead of any directive as the name would suggest?

something like hasOptionalDirective or so?

brettjurgens and others added 30 commits June 7, 2018 05:42

fragment experimentation

982b589

add missing files

5737878

start IR and debug

23dcc53

flattening improvements & flatten adjacent fragments

0c22684

improvements to IR

3da685e

it works

4127e54

some type fixes & documentation

350b656

refactor interface

bcd9d0a

getting somewhere

ad6013c

fix build errors

3e9f901

Merge branch 'master' into experiment_with_fragments

35c165f

working!?

12ee5e9

fix nullish

77128b4

maybe fix grabbing field

8b444a6

fix fragment typecondition in flattening

5d9907d

rm unused import

79abe85

allow aliasing __typename

b50059f

subtype generation

aa4f7cb

start integrating in fromQuery

aea052f

sort in new array instead

4672791

postprocess whole output

905b1da

list override

4d790e7

export stuff

b353a56

spread var interfaces

b1e43c8

pass user options in

4807d00

fix non-null

95b0b3b

v2.0.0-0

27382fc

fix input type

183b56e

v2.0.0-1

34707b8

some cleanup

ffa83da

v2.0.0-3

6e85dcd

BrandonHermanAvant requested review from Michael-Place and zhejia0428 March 11, 2020 20:05

BrandonHermanAvant commented Mar 11, 2020

View reviewed changes

brettjurgens reviewed Mar 11, 2020

View reviewed changes

BrandonHermanAvant changed the title ~~Cf 897 fix directives~~ CF-897: Fix directives in v2.0.0-3 Mar 13, 2020

Mike Place and others added 4 commits March 13, 2020 12:20

WIP

162da04

CF-897: Fix directives

dc50c7a

Clean up

402c649

CF-897: Use OPTIONS.formatInput

beaeb8a

BrandonHermanAvant force-pushed the CF-897_fix_directives branch 2 times, most recently from 8361abe to beaeb8a Compare March 13, 2020 17:24

bump tests

8271be8

BrandonHermanAvant changed the base branch from v2.0.0-3 to experiment_with_fragments March 13, 2020 18:32

BrandonHermanAvant added 6 commits March 13, 2020 14:03

Merge branch '2.0.0' into experiment_with_fragments

6d46819

Fix tests

a814433

Comment out Union Tests

5226eb5

Clean up tests / comment out failures

e0bc678

Add testurl

83ffb4a

Merge branch 'experiment_with_fragments' into CF-897_fix_directives

e759f08

BrandonHermanAvant changed the base branch from experiment_with_fragments to 2.0.0 March 13, 2020 20:51

Merge branch '2.0.0' into CF-897_fix_directives

246d061

BrandonHermanAvant linked an issue Mar 13, 2020 that may be closed by this pull request

Roadmap to 2.0 #163

Open

17 tasks

BrandonHermanAvant changed the title ~~CF-897: Fix directives in v2.0.0-3~~ CF-897: Fix directives Mar 13, 2020

v2.0.0-4

854e639

BrandonHermanAvant force-pushed the CF-897_fix_directives branch from 71f1c6d to 854e639 Compare March 13, 2020 22:50

BrandonHermanAvant merged commit 75edcc9 into 2.0.0 Mar 13, 2020

BrandonHermanAvant deleted the CF-897_fix_directives branch March 13, 2020 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CF-897: Fix directives #267

CF-897: Fix directives #267

BrandonHermanAvant commented Mar 11, 2020 •

edited

Loading

BrandonHermanAvant Mar 11, 2020

BrandonHermanAvant Mar 11, 2020

brettjurgens left a comment

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

BrandonHermanAvant Mar 12, 2020

brettjurgens Mar 11, 2020

BrandonHermanAvant Mar 12, 2020 •

edited

Loading

brettjurgens Mar 11, 2020

BrandonHermanAvant Mar 12, 2020 •

edited

Loading

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

brettjurgens Mar 11, 2020

	const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives))
	const fragmentsHaveDirectives = selection.fragments.some(frag => hasDirectives(frag.directives))

	!!directives['include'] \|\| !!directives['skip']
	!!directives.include \|\| !!directives.skip

CF-897: Fix directives #267

CF-897: Fix directives #267

Conversation

BrandonHermanAvant commented Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettjurgens 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

BrandonHermanAvant Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrandonHermanAvant Mar 12, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrandonHermanAvant commented Mar 11, 2020 •

edited

Loading

BrandonHermanAvant Mar 12, 2020 •

edited

Loading

BrandonHermanAvant Mar 12, 2020 •

edited

Loading