Conversation
| 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; | ||
| } |
There was a problem hiding this comment.
This is the one new test I added
| it('works with inline fragments on base', () => { | ||
| const response: string = runProgram( | ||
| schema, | ||
| inlineFragmentWithDirectiveOnBaseQuery, | ||
| undefined, | ||
| { generateSubTypeInterfaceName } | ||
| ); | ||
| expect(response).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
New test
brettjurgens
left a comment
brettjurgens
left a comment
There was a problem hiding this comment.
unsolicited feedback 😅
packages/from-query/src/generate.ts
Outdated
| private buildDeclaration (selection: Selection): string { | ||
| switch (selection.kind) { | ||
| case 'Field': | ||
| const foo = !!selection.directives['include'] || !!selection.directives['skip'] |
There was a problem hiding this comment.
can we find a better name than foo 😄
There was a problem hiding this comment.
can we reuse the hasDirectives above? we can then do it inline below
{
// ...
optional: hasDirectives(selection.directives)
}There was a problem hiding this comment.
ooh good catch, got excited that I got it working and missed this cleanup spot lol
| // } | ||
| // }`) | ||
| // ).toMatchSnapshot(); | ||
| // }); |
There was a problem hiding this comment.
why are these tests commented out?
There was a problem hiding this comment.
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!
| ); | ||
| expect(response).toMatchSnapshot(); | ||
| }); | ||
| // it('supports unions', () => { |
There was a problem hiding this comment.
do unions no longer work? why are these commented out?
There was a problem hiding this comment.
Currently getting the following error:
TypeError: Cannot read property 'type' of null
packages/from-query/src/generate.ts
Outdated
| ); | ||
|
|
||
| const selectionHasDirectives = hasDirectives(selection.directives); | ||
| const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
packages/from-query/src/generate.ts
Outdated
| ); | ||
|
|
||
| const selectionHasDirectives = hasDirectives(selection.directives); | ||
| const fragmentsHaveDirectives = !!selection.fragments.some(frag => hasDirectives(frag.directives)) |
There was a problem hiding this comment.
| 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
packages/from-query/src/generate.ts
Outdated
| private buildDeclaration (selection: Selection): string { | ||
| switch (selection.kind) { | ||
| case 'Field': | ||
| const foo = !!selection.directives['include'] || !!selection.directives['skip'] |
There was a problem hiding this comment.
can we reuse the hasDirectives above? we can then do it inline below
{
// ...
optional: hasDirectives(selection.directives)
}
packages/from-query/src/generate.ts
Outdated
|
|
||
| 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'] |
There was a problem hiding this comment.
| !!directives['include'] || !!directives['skip'] | |
| !!directives.include || !!directives.skip |
you can use dot-notation
packages/from-query/src/generate.ts
Outdated
| }; | ||
|
|
||
| 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 => |
There was a problem hiding this comment.
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?
BrandonHermanAvant
changed the title
BrandonHermanAvant
force-pushed
the
CF-897_fix_directives
branch
2 times, most recently
from
8361abe to
beaeb8a
Compare
BrandonHermanAvant
changed the base branch from
v2.0.0-3
to
experiment_with_fragments
BrandonHermanAvant
changed the base branch from
experiment_with_fragments
to
2.0.0
BrandonHermanAvant
changed the title
BrandonHermanAvant
force-pushed
the
CF-897_fix_directives
branch
from
71f1c6d to
854e639
Compare
2 participants
This updates fixes directives in the current v2 branch.