Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug in @graphql-tools/wrap #1954

Closed
maapteh opened this issue Aug 26, 2020 · 12 comments
Closed

bug in @graphql-tools/wrap #1954

maapteh opened this issue Aug 26, 2020 · 12 comments

Comments

@maapteh
Copy link

maapteh commented Aug 26, 2020

This is a follow up of issue created at Urigo/graphql-modules#1295

I still have to understand how, but wanted to create the ticket as soon as possible.

@yaacovCR
Copy link
Collaborator

Hi!

@maapteh are you able to recreate this bug with just a regular sdl schema?

Is this a bug with wrap or just the way wrap works?

@kamilkisiela
Copy link
Collaborator

kamilkisiela commented Aug 26, 2020

@yaacovCR @maapteh #1955

I guess it's one execution per each root-level field so we need to have a dataloader on top or something similar.

@kamilkisiela
Copy link
Collaborator

It happens because we wrap all root-level resolvers with defaultCreateProxyingResolver and call delegateToSchema on resolver level and it's delegateToSchema that runs executor function.

@yaacovCR
Copy link
Collaborator

Should be fixed by batched executor which is (loosely) planned for v7, to be based on Gatsby-style batch execution

This right now is working as designed as far as I can tell but there is room of course for design improvement.

Batch execution is one solution, maybe there are others?

@yaacovCR
Copy link
Collaborator

Of course there already is HttpLinkDataLoader which batches as one request not necessarily one operation

@yaacovCR
Copy link
Collaborator

Actually, I am not 100% on whether batch execution fixes this issue.

The executor will still be called multiple times, so similar test may fail, but because multiple root fields or multiple operations can be sent as one, there will be only one network request. Would that solve this issue?

@maapteh
Copy link
Author

maapteh commented Aug 27, 2020

When the testcase for #1955 succeeds we know if it will solve it :)

@yaacovCR
Copy link
Collaborator

Why do you need the executor to be called once if it uses Data loader to batch and send one request?

@maapteh
Copy link
Author

maapteh commented Aug 27, 2020

Because now our dataloader is not called in the same request/operation. Dataloader only works when it happens in the same tick. But i think its a bug in combination apollo and our used modules and will go back there!

thanks Yaacov. Sorry for reporting here :)

@kamilkisiela
Copy link
Collaborator

The point is of that issue is that when you wrap the schema with wrapSchema you run executor for each root-level field, which means executing multiple GraphQL operations instead of single one.

@yaacovCR
Copy link
Collaborator

Seems to have been addressed within graphql-modules as documented in linked issue above. Closing this issue for now!

When query batching lands, see #1965, another possible workaround may be available.

yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info.

TODO:
- Add testing!
- Extensions support should be added by a custom option?

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info.

TODO:
- Add testing!
- Extensions support should be added by a custom option?

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
@yaacovCR yaacovCR mentioned this issue Aug 31, 2020
Merged
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info.

TODO:
- Add testing!
- Extensions support should be added by a custom option?

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954

fix
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add additional testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add additional testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add additional testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
WIP:

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

TODO:
- Add additional testing!

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
yaacovCR added a commit that referenced this issue Aug 31, 2020
When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954
ardatan pushed a commit that referenced this issue Sep 1, 2020
* enable batch execution

When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests.

Adapted from Gatsby query batcher by @vladar.

Caveats:
* Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen!
* Passed `info` argument from first executor call to the batched executor call, making info argument unreliable.

Related:

gatsbyjs/gatsby#22347 (comment)
#1710 (comment)
#1959 (comment)
#1954

* fix(delegate): pass extensions to executor/subscriber
@ardatan
Copy link
Owner

ardatan commented Sep 1, 2020

Available in 6.2.0!

@ardatan ardatan closed this as completed Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants