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

stitchSchemas and schema delegation lose(doesn't forward) operatioName #2884

Closed
Aliaksei-Martsinkevich opened this issue Apr 29, 2021 · 8 comments
Labels
stage/1-reproduction A reproduction exists

Comments

@Aliaksei-Martsinkevich
Copy link

Describe the bug
When I execute operation on stitchSchemas it doesn't forward operationName to underlying schemas. We use operationName for logs.

To Reproduce
Steps to reproduce the behavior:

https://codesandbox.io/s/naughty-franklin-b4cwd?file=/src/index.ts

Expected behavior
Operation name should forwarded to underlying schemas.

Additional context
Found this pr but it only fixed delegateToSchema;

#1700

@Urigo
Copy link
Collaborator

Urigo commented Apr 29, 2021

Thank you for reporting this issue @Aliaksei-Martsinkevich !

I'm not adding a lot here but just labeling it according to our new Contribution Guide and issue flow.

It seems already got into stage 1 thanks to your reproduction! Thank you for that!

Now in order to advance to stage 2 we'll need a failing test or reproduce it in the unit tests somehow. I believe this can be added somewhere in https://github.com/ardatan/graphql-tools/blob/master/packages/stitch/tests/stitchSchemas.test.ts

Would be great if someone could help progress the issues through the stages.

Thank you and sorry that this comment is not a complete solution (yet).

@Urigo Urigo added the stage/1-reproduction A reproduction exists label Apr 29, 2021
@yaacovCR
Copy link
Collaborator

#1924 (comment)
#2744 (comment)

Note that I think in the next major release we should go back to the old behavior of automatically forwarding the operationName and it can be overridden if necessary using the same type of workaround.

In theory, @ardatan we could even release it as a bug fix, although it is breaking, I doubt anybody is actually relying on the operationName not being identical -- my argument WAS that in theory someone might be relying on just the operationName for query caching, but I have not yet seen anybody in the wild actually doing that.

Or, we could just wait until the next major version, which I imagine will be with the next release of graphql-js?

@ardatan
Copy link
Owner

ardatan commented Apr 30, 2021

@yaacovCR we can do that for sure but what would happen if send multiple queries to the same source within the same operation?

@yaacovCR
Copy link
Collaborator

I guess that was my original concern, but even if identical, I guess adds helpful info as to ultimate source in Client, people don’t seem to care, and current behavior of dropping by default while technically more correct, seems to be unhelpful. I am on the fence about it, I think I should defer to you or anybody with more production experience actually using operation name

@ardatan ardatan changed the title stitchSchemas lose operation name stitchSchemas and schema delegation lose operation name May 11, 2021
@ardatan ardatan changed the title stitchSchemas and schema delegation lose operation name stitchSchemas and schema delegation lose(doesn't forward) operatioName May 11, 2021
@klippx
Copy link
Contributor

klippx commented Mar 10, 2022

Moving the comment from #4293 to here:

  • Are we saying we want to keep eating up operation name?
  • Are we saying we want people to fall into this trap, get confused, find this issue and realize they are forced to use createProxyingResolver + delegateToSchema to get this working, which feels like it should be default behaviour?

Even if we have multiple queries to same source to the same operation I think we should still forward the operation name as it was provided by the client. This is essentially what you are doing already, although you have chosen undefined as the common operation name instead of what the client provided.

I guess adds helpful info as to ultimate source in Client, people don’t seem to care, and current behavior of dropping by default while technically more correct, seems to be unhelpful. I am on the fence about it, I think I should defer to you or anybody with more production experience actually using operation name

I think this comment by @yaacovCR is on point. IDK if I agree that it is technically more correct, but I will trust you on that. But both from client perspective and from remote schema server perspective it is way more useful to have an operation name than to have compassion and understanding for what is "technically correct" 🧑‍🔬

The client can worry about splitting the operation up if necessary.

@klippx
Copy link
Contributor

klippx commented Mar 10, 2022

Updated code sandbox with workaround for graphql-tools v8:

https://codesandbox.io/s/bold-sound-cxjbiq?file=/index.js

@AlexeyRaga
Copy link

With #4321 merged, should the issue be solved? Or some other steps are required?
I get some remote schemas registered (using introspectSchema) but I don't see operationName to be passed through.
What am I missing?

@ardatan ardatan closed this as completed Mar 29, 2023
@ardatan
Copy link
Owner

ardatan commented Mar 29, 2023

@AlexeyRaga Could you create a new issue with a reproduction if the issue still remains? Thanks! Sorry for the late response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/1-reproduction A reproduction exists
Projects
None yet
Development

No branches or pull requests

6 participants