introduce GraphQLField, GraphQLInputField, GraphQLArgument, and GraphQLEnumValue #4288
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
yaacovCR
force-pushed
the
coordinates-without-coordinates
branch
from
fdf0223
to
f2d03c4
Compare
yaacovCR
added
the
PR: breaking change 💥
|
updated description above and label to indicate that this is a BREAKING CHANGE |
| @@ -222,7 +226,7 @@ export function experimentalGetArgumentValues( | |||
| // execution. This is a runtime check to ensure execution does not | |||
| // continue with an invalid argument value. | |||
| throw new GraphQLError( | |||
| `Argument "${argDef.name}" of required type "${argType}" was not provided.`, | |||
| `Argument "${isArgument(argDef) ? argDef : argDef.name}" of required type "${argType}" was not provided.`, | |||
There was a problem hiding this comment.
Why would this not be an argument? This check feels a bit redundant given the parent check where we check whether it in fact is a required argument
There was a problem hiding this comment.
Did a first pass, the change is rather large. What is driving this change, why have we not used the underlying branch, ... not sure I get the why.
This feels like quite a large breaking change, however I doubt people were using these non-classes extensively. The thing I however fear is that libraries like pothos/nexus/... might now be faced with a need for GraphQLField which might end up being a very large refactor.
| @@ -272,7 +276,7 @@ export function experimentalGetArgumentValues( | |||
| valueNode, | |||
| argType, | |||
| (error, path) => { | |||
| error.message = `Argument "${argDef.name}" has invalid value${printPathArray( | |||
| error.message = `Argument "${isArgument(argDef) ? argDef : argDef.name}" has invalid value${printPathArray( | |||
There was a problem hiding this comment.
Similar question here but I guess it could be a VariableSignature? In which case argDef seems like an inappropriate name
There was a problem hiding this comment.
Yes, it could be a variable signature, which is "a kind of argument definition", but confusing. We should probably rename argDef, but I would guess that should be in a separate PR.
I think you are right to push back over here. Essentially, the reason the changes from #3145 were historically not taken up, was because the PR seemed even huger, with many changes to the error messages and the associated test files. I have been trying to break down the PR into smaller chunks, namely #4177 and this PR. Basically, what I am attempting to do is separate #3145 from the PR it depends on #3044, which has to do more broadly with integrating schema coordinates into the grammar and utility set, and just focusing on improving the error messages. I chose to keep the portions of #3145 that introduce the new classes, but instead of using a class hierarchy and directly referencing the "coordinate," instead just generally introduce a new property linking the schema element to its parent (fields to types, arguments to fields, etc) so that the PR could be more generic. I can make this "less breaking" by simply introducing a new computed value (the coordinate or the parent) on the fields/arguments/enum values without introducing the new classes, but it's also a breaking change, as libraries like pothos and nexus would still have to be very aware of these new fields. So I considered introducing these new classes to be in some sense a better signpost that some underlying changes had happened than introducing a subtle new field that might be missed when upgrading. I might have to investigate more how pothos and nexus might be impacted to see which would make more sense. Considering the change is breaking and the gain in functionality (better error messages) is not huge, we could consider just altogether foregoing this change. But if we do make a breaking change, v17 would be a good time for it. Maybe best to discuss at the next wg, do more research, etc. |
This comment has been minimized.
This comment has been minimized.
@yaacovCR The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
|
Note: I've investigated this in a general way by attempting to upgrade pothos to use v17, specifically, the canary version within this PR generated above. I have essentially no significant downstream effects from this change, because I have noticed quite a few breaking changes around the changes to default values:
This is overall not super tricky to upgrade, but, as expected, requires upgrades of dependencies. Notably, within Tagging @hayes just in case the above is of interest |
|
thanks @yaacovCR for taking the time to explore upgrade process! I am curious about defaultValue change. Is there a way to serialize input values from their parsed versions (so that we can keep this backwards compatible in Pothos). One issue I'll have to figure out is that Pothos doesn't currently know anything about the external types, so we'd probably need to have a way to configure that for every scalar in order to make that work correctly. I'm happy to hear overall the upgrade looks pretty straight forward (outside some deps used in tests) |
this extracts logic from #3044 and #3145 (later rebased as #3807 and #3808) to implement more informative error messages without implementing the full schema coordinate RFC
This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with
String(<schemaElement>JSON.stringifu(<schemaElement>and.toString()and.toJSON()