-
-
Notifications
You must be signed in to change notification settings - Fork 798
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
Transforms for normalizing stitched subschema deprecations #1925
Conversation
Canary is still failing... otherwise, I think this is ready for another look. I just went fully generic throughout the entire transformation chain. |
Oh, question: is stitch the best package for these or is there a strong case for keep them generic in wrap? I’m on the fence. The remove-only behavior feels extremely specific to this stitching usecase. But, wrap is certainly more conventional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
Currently it does a strict equal comparison which will fail for input object types...
I do wonder if it would be better if the match directive function took the schema so that it could properly parse even custom scalars or whatever types that the directive arguments throw at it.
This could be easily implemented with getDirectives
https://github.com/ardatan/graphql-tools/blob/master/packages/utils/src/get-directives.ts#L57
Maybe two versions a la valueFromASTUntyped and valueFromAST from graphql-js?
Thoughts?
One note: these new transforms don’t touch input object fields. I can't think of a practical scenario where inputs should be groomed in this manner as they’re not a direct concern of the readable schema; also they don't support deprecation. Does that seem reasonable? |
Ooooooohh... I get it now. The AST is all strings, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for the transforms would also be helpful.
This is what I had in mind: |
handle repeatable directives, properly parsing directive arguments
…o gm-stitch-deprecations
lots more tests are in. |
OKAY – another big retooling. There are now 5 new transforms in the
Only the |
Wow! I guess the only last nit is that these transforms only work on object fields and not input object fields, so maybe they should have ObjectField in the name like FilterObjectFieldDirectives, etc Pretty long name, but descriptive, what do you think? And DS Store ended up in diff... Really looks great!!!! |
|
The latest changes of this PR are available as alpha in npm: Quickly update your package.json by running:
|
Adds package support and a documented approach for handling Federation-style stitching patterns that result in field oddities outside of the gateway context. While deprecation is an imperfect solution, it seems pretty reasonable as an approach. Full explanation in updated docs:
@yaacovCR
TODO: