-
Notifications
You must be signed in to change notification settings - Fork 222
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
Support different arguments for different aliases #482
Conversation
According to the GraphQL spec, the |
0010f49
to
e41f974
Compare
Just rebased on v3.3.2. Any chance of moving this forward? |
@bueche can you take a look into this one? |
@kryops This week I become maintainer of this project and I would love helping you merge this PR if you are still interested. Let me know, have a great weekend! |
docs/CHANGELOG.md
Outdated
following: { | ||
// ... | ||
resolve: (source, args, context, info) => { | ||
const rawValue = defaultFieldResolver(source, args, context, info) |
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.
assuming this is only required for "non-trivial" field usage. Please confirm. Also, perhaps it's useful to explain in more detail how this helps. Isn't this defaultFieldResolver()
called under-the-hood?
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, defaultFieldResolver
is only required for non-trivial fields.
The problem is that GraphQL only uses its defaultFieldResolver
if you do not specify a custom resolve
function. As soon as you do, you need to handle this yourself.
I extended the section on custom resolvers in the docs and added a link to it from the changelog, please let me know if this covers it better now 😅
docs/CHANGELOG.md
Outdated
// ... | ||
resolve: (source, args, context, info) => { | ||
const rawValue = defaultFieldResolver(source, args, context, info) | ||
return processUsers(rawValue) |
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.
and what do you mean by "rawValue" here? raw vs. what??
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.
Good point, it's just the value of the field - changed it to value
docs/warnings.md
Outdated
|
||
The `source.following` property is now a function that will return the correct value. GraphQL's default resolver will detect the function and do the right thing. | ||
|
||
Therefore, we recommend getting the raw value through GraphQL's default resolver first when using a custom resolver on a non-trivial field: |
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.
- "non-trivial field" ... this should have a more precise definition at least as best as you understand it.
- "recommend" ? ... perhaps too softly worded. What happens if you don't use the default resolver ... and (again) why are we calling the default resolver explicitly when it was previously called implicitly?
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.
True, it might throw an error or do weird things if you don't use the default resolver and a query uses multiple aliases, because source[fieldName]
would contain a function instead of the field value.
I changed the wording to "need to", and added a section explaining "non-trivial" values in more detail.
@@ -275,6 +275,9 @@ const User = new GraphQLObjectType({ | |||
}, | |||
writtenMaterial1: { | |||
type: new GraphQLList(AuthoredUnion), | |||
args: { | |||
search: { type: GraphQLString }, |
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.
Excellent to add to the tests ... can you explain what is being added to test this?
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 was done to be able to test multiple aliases with different args on union and interface types. I added a comment explaining this.
Hi all, yes I'm still interested in getting this merged. Thanks for the feedback, I'll try to find some time this week to address it 😅 |
I'm planning to do a v3.3.5 with the small fixes currently in master and then create a v3.4.0 release with this. |
Should this rather be a v4.0.0 as it is a breaking change for custom resolvers? |
I understood it is only a breaking change for users that are using broken aliases but yeah we can for sure make it v4.0.0 |
hi Nico
will try to look at it soon.
cheers
Ed
…On Sat, May 4, 2024 at 11:54 AM Nico Gallinal ***@***.***> wrote:
@kryops <https://github.com/kryops> This week I become maintainer of this
project and I would love helping you merge this PR if you are still
interested.
Let me know, have a great weekend!
—
Reply to this email directly, view it on GitHub
<#482 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJTEHDK3Q5RLRS5KHKWLZ2LZAUVHHAVCNFSM5WFMAHH2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBZGQZTINRUGEZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
PR has been opened for too long. I went over it and it LGTM. It has nice testing suite and code is clean a documented.
I don't want to delay this further, thanks a lot @kryops for you patience and commitment!
docs/CHANGELOG.md
Outdated
|
||
**Breaking changes:** | ||
|
||
It is no longer guaranteed that a field's value is available under `source[fieldName]` in a custom resolver. Instead, custom resolvers on non-trivial fields need to use GraphQL's default resolver to get the field value: |
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.
can we change "non-trivial" for "fields with multiple aliases"?
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.
Not sure - whether or not there are multiple aliases depends on the client query, so the schema should probably assume this might always be the case.
What I wanted to express with "non-trivial" is "types of fields that handle multiple aliases this way because they might return different data for each of them" 😅
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.
Ah ok then link to the definition of non trivial so people reading the changelog know what it means 😀
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.
Done
docs/warnings.md
Outdated
|
||
The `source.following` property is now a function that will return the correct value. GraphQL's default resolver will detect the function and do the right thing. However, passing a custom resolver will completely override GraphQL's default resolver instead of wrapping it. | ||
|
||
Therefore, it is required to get the actual value through GraphQL's default resolver first when using a custom resolver on a non-trivial field: |
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.
Link to definition
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.
Done
docs/warnings.md
Outdated
|
||
The `source.following` property is now a function that will return the correct value. GraphQL's default resolver will detect the function and do the right thing. However, passing a custom resolver will completely override GraphQL's default resolver instead of wrapping it. | ||
|
||
Therefore, it is required to get the actual value through GraphQL's default resolver first when using a custom resolver on a non-trivial field: |
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.
Link to definition
Hi there,
after experimenting with different workarounds in #481, I believe I may have found a proper fix for #126.
Please let me know what you think☺️
I published the changes to npm as
@kryops/[email protected]
if anyone wants to try it out with their schema.Description
The general strategy is as follows:
join-monster
tries to detect conflicting aliases to the same fieldfieldName
:The query
generates per user:
GraphQL's default resolver supports functions as properties, and will call them with the information necessary to return the correct property.
This is a breaking change when using custom resolvers: Instead of always accessing
source[fieldName]
, GraphQL'sdefaultFieldResolver
has to be used to get the raw value in order to support multiple conflicting aliases:Note that this should be only actually breaking in situations that likely used to return wrong data. For normal fields and non-conflicting aliases the same format is used as before.
References
Fixes #126
Fixes #146
Checklist
join-monster
does not use the args, thus they should point to the same data anyway (e.g. normal columns)sqlExpr
), the args may be used byjoin-monster
, so aliases are conflicting if they use different argscomments
andcomments$
?Post.comments
orUser.comments
?writtenMaterial1
orwrittenMaterial2
?)