-
Notifications
You must be signed in to change notification settings - Fork 16
New Generic Field Rewriter & Custom Rewriter #51
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
base: master
Are you sure you want to change the base?
New Generic Field Rewriter & Custom Rewriter #51
Conversation
…phql-query-rewriter-core into feature/ap/field-rewriter
Apologies for the delay getting this reviewed, I'm traveling this week so it might take a few more days before I can get this reviewed and merged. I'll aim to do so by early next week at the latest. Thank you so much for submitting this! It looks like a great improvement. |
/** | ||
* More generic version of ScalarFieldToObjectField rewriter | ||
*/ | ||
class FieldRewriter extends Rewriter { |
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 looks like it's doing the same thing as ScalarFieldToObjectFieldRewriter
, but with the option of rewriting the field name. If that's correct, would it make sense to add a FieldNameRewriter
instead which can rename a field, and then that could be used in series with the ScalarFieldToObjectFieldRewriter
rather than trying to duplicate functionality? I'm worried that trying to put too much functionality into a single rewriter will make it confusing what that rewriter is doing.
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.
So, in addition to what the ScalarFieldToObjectFieldRewriter
does, the FieldRewriter not only also renames the field, but it supports tapping into its arguments too. I guess Im down for the approach of having more limited rewriters. So for that we'd probably need two additional ones: FieldNameRewriter
and FieldArgsRewriter
.
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.
There's already FieldArgNameRewriter
and FieldArgTypeRewriter
for rewriting args. Would anything else besides those be needed?
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.
So, adding arguments to a Field that has no initial arguments is missing, also argument value coercion I think is also lacking.
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.
FieldArgTypeRewriter
should be able to do argument value coercion. What's the use case for adding an arg to a field? adding an argument to a field isn't a breaking change
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.
You're right FieldArgTypeRewriter has argument coercion.
About the other thing, It shouldn't be a breaking change to add args to a field with no initial args, as its optional. Maybe my use case is somewhat specific, but I used to have a resolver that based on a scalar field it made a totalCount aggregation returning the total number of results. Now I dont have that resolver anymore so I need to convert that scalar field into a object field but also passing a filter argument (there's a test for this in the rewriteField.test.ts file called 'rewrites a scalar field to be a renamed object field with variable arguments and with 1 scalar subfield').
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.
aah interesting I see what you mean about maybe needing to add an argument to make an old query valid in a new schema. That is a use-case that none of the current rewriters handle currently. Philosophically this library has favored small targeted rewriters for each use-case rather than 1 big one that does most things, but maybe this is a good change actually. For adding arguments, what do you think about making it a map instead of an array? ex arguments: { arg1: "$arg1" }
instead of arguments: ['arg1']
to give more control over what the argument value is set to? I can imagine a situation where you might want to set it to a constant or something, like arguments: { perPage: 1 }
.
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.
Yes totally, that sounds even better
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.
Aside from the arguments convo, could you also add some brief documentation about how the FieldRewriter
works to the README.md
? Maybe for now we can label it as 'EXPERIMENTAL' or something like that. Probably it will make sense to do a new major-version update in the future where we deprecate the old rewriters in favor of your FieldRewriter
. Other than that, should be good to go!
Thank you so much for these changes! This is such a great update!
@chanind Ive added a new Rewriter:
FieldRewriter
inspired by theScalarFieldToObjectFieldRewriter
and aCustomRewriter
to allow custom implementation of the Rewriter public methods.PR should be backwards compatible and comes with unit tests.
New functionalities:
ScalarFieldToObjectFieldRewriter
, but it can also rename the field in question.nodeAndParent
context variable to rewriteResponse fn: Useful for response rewrites that need to know what the query was initially (e.g, for__typename
response rewrites).