Ensure error resolving an entity don't impact resolvable ones (for version-0.x branch)
#1305
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@trevor-scheer wouldn't mind your opinion here since you followed earlier iterations of this. |
trevor-scheer
approved these changes
Dec 16, 2021
Comment on lines
+1679
to
+1702
| addResolversToSchema(serviceMap['S1'].schema, { | ||
| T1: { | ||
| __resolveReference(ref) { | ||
| return s1_data[ref.id]; | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| addResolversToSchema(serviceMap['S2'].schema, { | ||
| Query: { | ||
| getT1s() { | ||
| return [{id: 0}, {id: 1}, {id: 2}]; | ||
| }, | ||
| }, | ||
| T1: { | ||
| __resolveReference(ref) { | ||
| // the ref has already the id and f1 is a require is triggered, and we resolve f2 below | ||
| return ref; | ||
| }, | ||
| f2(o) { | ||
| return o.f1 === null ? null : { a: `t1:${o.f1}` }; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Nothing wrong here, but these resolvers can be added to the fixture object itself i.e.
const s1 = { name, typeDefs, resolvers };
Fixes apollographql#376 Co-authored-by: Sylvain Lebresne <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rebased and modified version of apollographql/apollo-server#3914. The differences with that earlier PR are that:
executeSelectionSetanymore: the reasoning (explained in a comment in the code) is that another error has already been logged and that error has more context, so adding a new error is redundant and I think it's confusing because that particular error lacks context and so doesn't really tell you what the problem is. So at best, you happen to know it's just a consequence of a previous error and ignore it, but better not have it then.return nullinstead of abreak: I personally think that it's unreasonable that when you require a non-nullable field then you may actually have to resolve without that required field. If you knew how to do that, then you wouldn't a@requirefor that field in the first place. So if you can't get all the requires of fetch (and thus, in particular, of a@require), then I think we should simply not do that fetch for the entity at all, which is whatreturn nulldoes. Usingbreakinstead means we'd attempt the fetch nonetheless but with only parts of the inputs present. Note that if a required field is nullable, then this code don't get trigger and we correctly do the fetch withnullvalues (I added tests that show it's the case).