`createRequest` with transforms API #724

mfix22 · 2018-04-09T17:32:23Z

@freiksenet I just recreated the createBatchOperation function using the new transforms schema, and

~~I am running into this error: "{ Error: Fields "bookingsByPropertyId" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional."~~

TODO:

More tests

…ls into apollographql-next-api

freiksenet

Awesome stuff 👍

freiksenet · 2018-04-10T07:30:51Z

src/stitching/delegateToSchema.ts

-      fragmentName => info.fragments[fragmentName],
-    ),
-    info.operation.variableDefinitions,
+export function createBatchOperation(


We can probably name it createDocument. createDocument was never a public API and this probably should be.

What do you think about createOperation? Since I think it's good to return the variables too.

freiksenet · 2018-04-10T07:32:15Z

src/stitching/delegateToSchema.ts

+  },
+  transforms?: Array<Transform>,
+): FetcherOperation {
+  const roots = Object.keys(rootDefs).map(key => {


Question here is whether we should auto-alias stuff or if we should expect user to alias themselves. I guess latter. Data-loading API can then alias by itself.

I was thinking users should add aliasing themselves:

{ 'user1:node': [{ id: 1 }, ...], 'user2:node': [{ id: 2 }, ...] }

But obviously with the API you specified below (:

freiksenet · 2018-04-10T07:33:59Z

src/stitching/delegateToSchema.ts

+  transforms?: Array<Transform>,
+): FetcherOperation {
+  const roots = Object.keys(rootDefs).map(key => {
+    const [args, info] = rootDefs[key];


Ok, this starts getting very complicated. How about this as a signature for rootdefs:

rootDefs: Array<{ alias?: string, fieldName: string, args?: { [key: string]: any }, info: DocumentInfo }>

Also extract it to a type.

Yeah that works for me!

freiksenet · 2018-04-10T07:34:10Z

src/stitching/delegateToSchema.ts

+  targetOperation: 'query' | 'mutation' | 'subscription',
+  rootDefs: { [key: string]: [{ [key: string]: any }, { [key: string]: any }] },
+  graphqlContext: { [key: string]: any },
+  documentInfo: {


Extract this to a type.

Disregard that. Let's always pass info through rootDefs. Having two sources of truth is confusing.

Also you don't have fieldNodes in documentInfo atm.

It's kinda complicated. You might need to add fragment aliasing. But then we probably don't want to duplicate identical fragments. Same with variables.

Yeah i always meant for that to be the full info object, but i just created a new type from the fields i was using. I'll update that 😄

freiksenet · 2018-04-10T07:39:03Z

src/stitching/delegateToSchema.ts

-    CheckResultAndHandleErrors(info, options.fieldName),
+  transforms = [
+    ...(transforms || []),
+    ...roots.map(({ args }) => AddArgumentsAsVariables(targetSchema, args)),


This will reset a variable counter each time. I think you should have one AddArgumentsAsVariables, it should be modified accept args lists with fieldName or alias and assign variables to them.

This way we don't need to traverse AST that many times.

@freiksenet do you think that parameter's type should be:

args: { [key: string]: any } | Array<{ fieldName: string, alias?: string, args: { [key: string]: any } }>

?
That seems to be the only way we can support both patterns right?

freiksenet · 2018-04-10T07:39:23Z

src/stitching/delegateToSchema.ts

+  transforms = [
+    ...(transforms || []),
+    ...roots.map(({ args }) => AddArgumentsAsVariables(targetSchema, args)),
+    FilterToSchema(targetSchema),


I'm pretty damn proud this still works without any problem :D

- Extract OperationRootDefinition type

mfix22 · 2018-04-11T18:20:42Z

src/stitching/delegateToSchema.ts

+  targetOperation: 'query' | 'mutation' | 'subscription',
+  rootDefs: Array<OperationRootDefinition>,
+  graphqlContext: { [key: string]: any },
+  documentInfo: GraphQLResolveInfo,


I left this for now, but let me know if you want me to change this. documentInfo was a prop i though users could use to define properties that are global to that operation, and do not affect specific root fields. Like fragments, for example.

mfix22 · 2018-04-12T18:16:30Z

src/stitching/delegateToSchema.ts

+  return {
+    query,
+    variables,
+    context: graphqlContext,


Should we return context: graphqlContext or context: { graphqlContext }? My original intention was the second because then you can take the return object and pass it directly to a fetcher.

Or should we remove context all together, and just have this function return { query, variables or { document, variables }?

Just document and variables I think.

freiksenet

We are nearly there.

Can you add tests for createOperation? Test it by running execute with it. I at least want to see one that has aliases, one that doesn't, different arguments etc.
Could you add API docs for createOperation to schema delegation docs?

freiksenet · 2018-04-17T08:21:10Z

src/stitching/delegateToSchema.ts

+  return {
+    query,
+    variables,
+    context: graphqlContext,


Just document and variables I think.

freiksenet · 2018-04-17T08:21:42Z

src/stitching/delegateToSchema.ts

+  };
+}
+
+export default async function delegateToSchema(


Pet peeve - could you move default export on top?

…ls into apollographql-next-api

freiksenet · 2018-04-18T10:52:06Z

As discussed, let's rename it to createRequest to match transforms API. Otherwise this is good. Once #527 is merged, we'll release that one as 3.0, merge this one and release it as @next pre-release for 3.1.

freiksenet · 2018-04-18T10:56:46Z

Oh yeah, and you need to export it in src/index.ts.

…ools into apollographql-next-api

benjamn · 2018-04-23T22:27:27Z

@mfix22 Can you rebase this PR against master now that #527 has been merged?

stubailo · 2018-07-31T19:22:35Z

@mfix22 so to recap, this is a way to do everything that delegateToSchema does except for actually sending the request, allowing you to do stuff like sending it in a batch?

Would this be better done via Apollo Link? Also, looks like there were some PRs around changing how subscriptions are done and similar, would you have a minute to update this if you think it's necessary?

mfix22 · 2018-07-31T19:34:18Z

@stubailo yes. The PR takes a function that delegateToSchema uses, extends it to allow for multiple root values, and exposes that function. That allows are options like creating a batches request, and sending it directly to to a remote server without using execute.

I think it fits best here, considering 90% of the logic is just used directly by delegateToSchema.

mfix22 · 2018-08-12T19:23:25Z

@stubailo do you need me to update anything here? 🙂

gigouni · 2018-10-08T14:53:20Z

Hi,

Is this still in the roadmap? It seems to be what I need to.

Thanks

ping @benjamn @freiksenet @mfix22

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR · 2020-03-27T12:46:48Z

Closing in favor of alternative implementation within #1307.

Mike Fix added 4 commits April 9, 2018 10:29

createBatchOperation with transforms API

f9c46be

Merge branch 'next-api' of git://github.com/apollographql/graphql-too…

33dcd04

…ls into apollographql-next-api

Fix tests for adjacent fields in fragments

db3a233

remove createDocument that is no longer used or exported

b3a97c6

freiksenet suggested changes Apr 10, 2018

View reviewed changes

Mike Fix added 2 commits April 10, 2018 17:03

WIP fix createOperation API (thanks freiksenet)

34d4eb5

- Extract OperationRootDefinition type

WIP: address more review comments

5552a3e

mfix22 commented Apr 11, 2018

View reviewed changes

Mike Fix added 3 commits April 12, 2018 10:38

Remove hack that will cause bugs

02db0a4

Fix tests

251d0dc

Clean up

8c5e3c8

mfix22 commented Apr 12, 2018

View reviewed changes

Mike Fix added 2 commits April 13, 2018 12:44

Fix bugs related to passing no arguments

a726453

Add two tests (let me know if i should move them)

a98d8cc

freiksenet suggested changes Apr 17, 2018

View reviewed changes

freiksenet mentioned this pull request Apr 17, 2018

WIP: createBatchOperation() #709

Closed

Mike Fix added 5 commits April 17, 2018 09:56

Return Request type, createOperation -> createDocument

6b28177

More tests for createDocument

242cde0

Merge branch 'next-api' of git://github.com/apollographql/graphql-too…

a8704ac

…ls into apollographql-next-api

Add execution test

fade3fb

createDocuments docs

4be53ed

freiksenet approved these changes Apr 18, 2018

View reviewed changes

Mike Fix added 2 commits April 18, 2018 08:50

createDocument -> createRequest

5bb040b

Export createRequest

6592810

mfix22 changed the title ~~createBatchOperation with transforms API~~ createRequest with transforms API Apr 18, 2018

Merge branch 'next-api' of https://github.com/apollographql/graphql-t…

11697ae

…ools into apollographql-next-api

Merge branch 'master' into apollographql-next-api

e657fae

Michael Fix and others added 3 commits June 18, 2018 11:41

Merge branch 'master' into apollographql-next-api

779100b

Merge branch 'master' into apollographql-next-api

fed0f01

Merge branch 'master' into apollographql-next-api

948355e

stubailo added the schema stitching label Jul 19, 2018

stubailo added the waiting on contributor label Jul 31, 2018

stubailo removed the waiting on contributor label Jul 31, 2018

mfix22 and others added 2 commits July 31, 2018 21:40

Merge branch 'master' into apollographql-next-api

a61afae

Merge branch 'master' into apollographql-next-api

ffd5f4c

Michael Fix and others added 2 commits August 29, 2018 10:06

Merge branch 'master' into apollographql-next-api

a8e5038

Merge branch 'master' into apollographql-next-api

2a8e82c

mfix22 added 2 commits October 15, 2018 12:27

Merge branch 'master' into apollographql-next-api

536805b

Merge branch 'master' into apollographql-next-api

e08cc10

mfix22 requested a review from hwillson October 23, 2018 19:44

Merge branch 'master' into apollographql-next-api

5607de4

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Jan 16, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

a665eb9

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Jan 21, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

39b8b2e

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Jan 21, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

cc4e16f

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Feb 17, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

2ddc73e

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Feb 27, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

38bdcea

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 26, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

4fe3153

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 26, 2020

feat(stitching): export createDelegatingRequest and delegateRequest m…

3fecf4e

…ethods. May be useful for use with dataloaders or memoization. See ardatan#724

yaacovCR mentioned this pull request Mar 27, 2020

merge graphql-tools-fork into v5 #1307

Merged

5 tasks

yaacovCR closed this Mar 27, 2020

yaacovCR mentioned this pull request Mar 27, 2020

v5 Roadmap #1306

Closed

22 tasks

mfix22 deleted the apollographql-next-api branch March 28, 2020 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`createRequest` with transforms API #724

`createRequest` with transforms API #724

mfix22 commented Apr 9, 2018 •

edited

Loading

freiksenet left a comment

freiksenet Apr 10, 2018

mfix22 Apr 10, 2018

freiksenet Apr 10, 2018

mfix22 Apr 10, 2018 •

edited

Loading

freiksenet Apr 10, 2018

freiksenet Apr 10, 2018

mfix22 Apr 10, 2018

freiksenet Apr 10, 2018

freiksenet Apr 10, 2018

freiksenet Apr 10, 2018

freiksenet Apr 10, 2018

mfix22 Apr 10, 2018

freiksenet Apr 10, 2018

freiksenet Apr 10, 2018

mfix22 Apr 11, 2018 •

edited

Loading

freiksenet Apr 10, 2018

mfix22 Apr 11, 2018

mfix22 Apr 12, 2018

mfix22 Apr 13, 2018 •

edited

Loading

freiksenet Apr 17, 2018

freiksenet left a comment

freiksenet Apr 17, 2018

freiksenet Apr 17, 2018

freiksenet commented Apr 18, 2018

freiksenet commented Apr 18, 2018

benjamn commented Apr 23, 2018

stubailo commented Jul 31, 2018

mfix22 commented Jul 31, 2018

mfix22 commented Aug 12, 2018

gigouni commented Oct 8, 2018

yaacovCR commented Mar 27, 2020

createRequest with transforms API #724

createRequest with transforms API #724

Conversation

mfix22 commented Apr 9, 2018 • edited Loading

freiksenet 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

mfix22 Apr 10, 2018 • 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

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

mfix22 Apr 11, 2018 • 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

mfix22 Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freiksenet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freiksenet commented Apr 18, 2018

freiksenet commented Apr 18, 2018

benjamn commented Apr 23, 2018

stubailo commented Jul 31, 2018

mfix22 commented Jul 31, 2018

mfix22 commented Aug 12, 2018

gigouni commented Oct 8, 2018

yaacovCR commented Mar 27, 2020

`createRequest` with transforms API #724

`createRequest` with transforms API #724

mfix22 commented Apr 9, 2018 •

edited

Loading

mfix22 Apr 10, 2018 •

edited

Loading

mfix22 Apr 11, 2018 •

edited

Loading

mfix22 Apr 13, 2018 •

edited

Loading