[graphql-tools/utils] Add graphql v17 args defaultValue support#8226
[graphql-tools/utils] Add graphql v17 args defaultValue support#8226eddeee888 wants to merge 18 commits into
Conversation
🦋 Changeset detectedLatest commit: e54830a The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GraphQL v17 compatibility: ChangesGraphQL v17 defaultValue Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@graphql-tools/utils': minor | |||
There was a problem hiding this comment.
I'm using minor here as supporting graphql v17 args defaultValue is a new feature (?).
Alternatively, since package.json already says peerDeps allows graphql@17 , this could be a patch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/utils/tests/getDocumentNodeFromSchema.spec.ts`:
- Around line 4-42: Add comprehensive tests in
packages/utils/tests/getDocumentNodeFromSchema.spec.ts to cover non-string
default value types for getDocumentNodeFromSchema: add cases for INT (input: Int
= 42), FLOAT (input: Float = 3.14), BOOLEAN (input: Boolean = true), ENUM
(input: MyEnum = VALUE), LIST (input: [String] = ["a","b"]), and OBJECT (input:
InputType = { field: "value" }). For each case call
getDocumentNodeFromSchema(schema), locate the argument via
result['definitions'].find(def => def.kind ===
Kind.OBJECT_TYPE_DEFINITION)?.fields?.[0].arguments and assert the defaultValue
has the correct kind (IntValue, FloatValue, BooleanValue, EnumValue, ListValue,
ObjectValue) and appropriate value/structure (e.g. value strings for numeric
kinds, boolean true for BooleanValue, matching name for EnumValue, array of
StringValue nodes for ListValue, and matching ObjectValue fields for input
object). Ensure tests run against both v16/v17 code paths by mirroring existing
test structure and imports (including Kind).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89d9892f-2904-4f88-9c67-bca148672e83
📒 Files selected for processing (4)
.changeset/eager-cities-relate.md.github/workflows/tests.ymlpackages/utils/src/print-schema-with-directives.tspackages/utils/tests/getDocumentNodeFromSchema.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/utils/tests/getDocumentNodeFromSchema.spec.ts (1)
43-388: ⚡ Quick winAdd one regression case for explicit
nullargument defaults.Great coverage expansion. Please add
something(input: String = null): Stringand assertdefaultValue.kind === "NullValue"so the v17 path is guarded against null-dropping regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/tests/getDocumentNodeFromSchema.spec.ts` around lines 43 - 388, Add a regression test in packages/utils/tests/getDocumentNodeFromSchema.spec.ts that mirrors the other default-value tests: build a schema with "type Query { something(input: String = null): String }", call getDocumentNodeFromSchema(schema), extract the argument definition from the OBJECT_TYPE_DEFINITION field (same pattern used in existing tests), and assert that the argument's defaultValue.kind equals "NullValue" (and/or match an inline snapshot showing kind "NullValue") to ensure null defaults aren't dropped in the v17 path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/utils/src/print-schema-with-directives.ts`:
- Around line 300-317: The code accesses arg.default?.literal which doesn't
exist on graphql@16's GraphQLArgument type; update astFromArg to guard/cast
before dereferencing: detect if (arg as any).default?.literal or check
Object.prototype.hasOwnProperty.call(arg, 'default') and read default via (arg
as any).default to get literal, then run convertConstValueNode on that value;
keep the rest of the conversion logic unchanged and only change the access to
use a safe cast/feature-detection for the `default` property.
- Around line 302-305: convertConstValueNode currently maps Kind.NULL to
undefined which later becomes omitted by astFromValue causing explicit null
defaults (e.g., arg.default.literal) and nested nulls to be dropped; update
convertConstValueNode so that when node.kind === Kind.NULL it returns null (not
undefined), and ensure any code paths that handle lists/objects (e.g., the
Kind.LIST branch using node.values.map(convertConstValueNode) and object
conversion) preserve null values, so the schema printer will emit "= null" and
nested nulls correctly.
---
Nitpick comments:
In `@packages/utils/tests/getDocumentNodeFromSchema.spec.ts`:
- Around line 43-388: Add a regression test in
packages/utils/tests/getDocumentNodeFromSchema.spec.ts that mirrors the other
default-value tests: build a schema with "type Query { something(input: String =
null): String }", call getDocumentNodeFromSchema(schema), extract the argument
definition from the OBJECT_TYPE_DEFINITION field (same pattern used in existing
tests), and assert that the argument's defaultValue.kind equals "NullValue"
(and/or match an inline snapshot showing kind "NullValue") to ensure null
defaults aren't dropped in the v17 path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a433056c-d9fa-4d4c-aaea-c62ce7fed300
📒 Files selected for processing (2)
packages/utils/src/print-schema-with-directives.tspackages/utils/tests/getDocumentNodeFromSchema.spec.ts
There was a problem hiding this comment.
This is so we can construct a similar info when executing resolvers
eddeee888
force-pushed
the
fix-graphql-17-args-default-value
branch
from
32b6504 to
1679aeb
Compare
eddeee888
force-pushed
the
fix-graphql-17-args-default-value
branch
from
1679aeb to
df38cf6
Compare
| promiseAll: values => exeContext.asyncWorkTracker.promiseAllTrackOnReject(values), | ||
| track: maybePromises => exeContext.asyncWorkTracker.addValues(maybePromises), | ||
| }), | ||
| } as GraphQLResolveInfo; |
There was a problem hiding this comment.
as GraphQLResolveInfo is needed to massage type difference between graphql@17 vs below.
WIihout this earlier versions will see type errors because GraphQLResolveInfo wouldn't have getAbortSignal and getAsyncHelpers
| "@graphql-tools/executor-common": "1.0.6", | ||
| "@graphql-tools/wrap": "11.1.16", |
There was a problem hiding this comment.
These external graphql-tools packages are required for tests to work when updating to v17:
@graphql-tools/batch-delegate@graphql-tools/executor-common@graphql-tools/wrap
There was a problem hiding this comment.
I got something like this in tests:
CombinedGraphQLErrors: Cannot use GraphQLSchema "{ __kind: Symbol(Schema), assumeValid: false, __validationErrors: [], description: undefined, extensions: {}, astNode: { kind: "SchemaDefinition", operationTypes: [Array] }, extensionASTNodes: [], _queryType: Query, _mutationType: Mutation, _subscriptionType: Subscription, _directives: [@include, @skip, @deprecated, @specifiedBy, @oneOf], _typeMap: { File: File, Query: Query, String: String, Mutation: Mutation, Subscription: Subscription, Boolean: Boolean, __Schema: __Schema, __Type: __Type, __TypeKind: __TypeKind, __Field: __Field, __InputValue: __InputValue, __EnumValue: __EnumValue, __Directive: __Directive, __DirectiveLocation: __DirectiveLocation }, _subTypeMap: {}, _implementationsMap: {} }" from another module or realm.
Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.
https://yarnpkg.com/en/docs/selective-version-resolutions
Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.
But when I removed these and only run tests for the @graphql-tools/utils package, the errors don't happen 🤔
I'll remove these for now here
eddeee888
force-pushed
the
fix-graphql-17-args-default-value
branch
2 times, most recently
from
e647a80 to
fe5e4e3
Compare
eddeee888
force-pushed
the
fix-graphql-17-args-default-value
branch
from
fe5e4e3 to
3c1ba55
Compare
enisdenjo
left a comment
enisdenjo
left a comment
There was a problem hiding this comment.
some nits, otherwise looking good! great work!
| "@graphql-tools/executor-common": "1.0.6", | ||
| "@graphql-tools/wrap": "11.1.16", |
| coercedValues[name] = defaultValue; | ||
| } else if (isNonNullType(argType)) { | ||
| const defaultValue = defaultValueFromType(arg); | ||
| if (defaultValue) { |
There was a problem hiding this comment.
truthiness check here silently drops valid defaults of 0, false, and ""
There was a problem hiding this comment.
defaultValueFromType should return AST | undefined here, so the falsy cases of 0, false, and "" will be wrapped in the AST objects.
I'll have a commit to rename the function from defaultValueFromType to defaultValueAstFromType to make it clearer
| } else if (isNonNullType(argType)) { | ||
| const defaultValue = defaultValueFromType(arg); | ||
| if (defaultValue) { | ||
| coercedValues[arg.name] = defaultValue.value; |
There was a problem hiding this comment.
defaultValueFromType returns two different shapes, and .value is only correct for one of them:
- v17
{ value }branch -> shim returns the rawGraphQLDefaultInputwrapper, so
.valueis the coerced runtime value - v17
{ literal: ConstValueNode }branch -> shim returnsastFromValue(...), a
ValueNode..valueon aValueNodeis the raw AST value ("888"instead of
888) orundefinedfor list/object defaults - graphql <17
defaultValuebranch -> shim returns aValueNode
is this expected?
There was a problem hiding this comment.
Good question 🤔 let me check/write the test cases
| * When runtime default value is available (in graphql@17), it'd return the object without Kind | ||
| * Otherwise, it'd return a `ValueNode`. | ||
| */ | ||
| export const defaultValueFromType = (arg: GraphQLArgument | GraphQLInputField) => { |
There was a problem hiding this comment.
can we provide typedefs for the expected return value here?
There was a problem hiding this comment.
The best we could do here is ValueNode | undefined for backwards compatibility, but it's a bit awkward as ConstValueNode should be used in v16 and 17 instead.
However, it exposed that the default case with value did not return a consistent interface. I updated the implementation here to keep it consistent with the other cases
| * Note: `node` is supposed to be `ConstValueNode` for graphql@17 but | ||
| * it is not available in graphql@15 so we cannot import it from `graphql` | ||
| */ | ||
| const convertConstValueNode = (node: any) => { |
There was a problem hiding this comment.
is this walker necessary? astFromValueUntyped already lives in this package and the codebase already uses it as the fallback in build-operation-for-field.ts. worth checking whether it (or graphql's own valueFromASTUntyped) covers this before maintaining a custom recursive converter (not sure, feel free to disregard)
| max_attempts: 5 | ||
| command: npm run ${{matrix.name == 'Leak' && 'test:leaks' || 'test'}} --ci | ||
|
|
||
| test-v17-temp: # FIXME: Gradual support for graphql@17, once every package is compatible, this should be merged back to main step |
There was a problem hiding this comment.
Not yet unfortunately, there are a few fixes we need to do other packages e.g. @graphql-tools/executor,
My plan is to release @graphql-tools/utils first to unblock Codegen, then come back and attempt to fix the rest
… and handle backward compatibility
2 participants
Description
Whilst working on dotansimha/graphql-code-generator#10866, I saw that args default value changes how it is presented as type:
More details in the v17 migration note: https://www.graphql-js.org/upgrade-guides/v16-v17/#default-values
This PR ensures the output of the conversion from type -> AST of args defaultValue stays the same.
Type of change
How Has This Been Tested?