[graphql-tools/utils] Add graphql v17 args defaultValue support#8226
Conversation
🦋 Changeset detectedLatest commit: 8f2306d 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
32b6504 to
1679aeb
Compare
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
There was a problem hiding this comment.
Actually, we need at least @n1ru4l/in-memory-live-query-store otherwise we'd get this error in some cases 🤔
Error: Patch file found for package in-memory-live-query-store which is not present at node_modules/@n1ru4l/in-memory-live-query-store
e647a80 to
fe5e4e3
Compare
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", |
| 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
ae0ddc4 to
38977ea
Compare
38977ea to
89d86ba
Compare
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?