graphql-tools

graphql-tools copied to clipboard

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;

https://github.com/ardatan/graphql-tools/pull/1700

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).

https://github.com/ardatan/graphql-tools/issues/1924#issuecomment-676607442 https://github.com/ardatan/graphql-tools/issues/2744#issuecomment-820738869

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?

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

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

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.

Updated code sandbox with workaround for graphql-tools v8:

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

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?

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