Add support for graphql-js v17 - #8313
Conversation
🦋 Changeset detectedLatest commit: ce9cd5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds GraphQL.js v17 compatibility across variable coercion, executor resolver metadata, scalar resolver configuration, package support, type patches, CI, and version-aware tests. ChangesGraphQL.js v17 compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Executor
participant GraphQLJSCompat
participant ResolverInfo
participant DirectiveEvaluation
Executor->>GraphQLJSCompat: convert execution variable values
GraphQLJSCompat-->>Executor: return GraphQL.js-compatible values
Executor->>ResolverInfo: provide variableValues, signal, and async helpers
Executor->>DirectiveEvaluation: evaluate stream directives with converted values
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/schema/src/addResolversToSchema.ts (2)
189-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
updateResolversInPlaceconsistent with the cloned-schema path.The in-place path updates
parseValue/coerceInputValue, butupdateResolversInPlace: truekeeps the existing scalar’s originalparseLiteraleven when resolver input coercing is overridden. Since execution falls back toparseLiteralwhen a scalar has nocoerceInputLiteral, remove/override the literal parser in the same scalar-override branch used by the mapped-schema path so both modes use the same new input coercer.🤖 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/schema/src/addResolversToSchema.ts` around lines 189 - 215, Update the scalar override branch in updateResolversInPlace so overriding input coercion also removes or replaces the existing scalar parseLiteral handler, matching the cloned-schema path. Ensure execution cannot retain the original parseLiteral when resolver input coercing is provided, so both schema update modes use the new coercer consistently.
313-354: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the inherited
coerceInputLiteralwhen overriding scalar coercion.
GraphQLScalarType.toConfig()carries the current scalar coercion methods, includingcoerceInputLiteral. If a resolver only overridesparseValue/coerceInputValuewithout also overriding literal coercion, leavecoerceInputLiteralon the copied config and literal argument coercion can still use the old resolver implementation. Remove it unless the resolver supplies a freshcoerceInputLiteral.🤖 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/schema/src/addResolversToSchema.ts` around lines 313 - 354, Update the scalar coercion cleanup after the resolver fields are copied so that when coerceInputValueOverridden is true and parseLiteralOverridden is false, it also removes the inherited coerceInputLiteral configuration. Preserve coerceInputLiteral when the resolver explicitly overrides literal coercion, using the existing detection symbols in the surrounding scalar setup.
🤖 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/getArgumentValues.ts`:
- Around line 52-73: Update the missing-variable handling in getArgumentValues
to reuse the existing graphql v17 arg.default coercion path when the referenced
variable has no coerced value, instead of checking only legacy arg.defaultValue.
Preserve the current coercion and invalid-default error behavior, while ensuring
field arguments referencing absent variables receive the v17 default before
non-null validation.
In `@packages/utils/src/graphqlJSCompat.ts`:
- Around line 95-104: Update buildVariableSources to preserve each variable’s
declared GraphQL input type instead of assigning anyValueType to every variable.
Pass variableDefinitions alongside variableValues from toGraphQLJSVariableValues
and build each source signature from the corresponding definition type,
preserving nested enum and custom-scalar literal/coercion behavior.
---
Outside diff comments:
In `@packages/schema/src/addResolversToSchema.ts`:
- Around line 189-215: Update the scalar override branch in
updateResolversInPlace so overriding input coercion also removes or replaces the
existing scalar parseLiteral handler, matching the cloned-schema path. Ensure
execution cannot retain the original parseLiteral when resolver input coercing
is provided, so both schema update modes use the new coercer consistently.
- Around line 313-354: Update the scalar coercion cleanup after the resolver
fields are copied so that when coerceInputValueOverridden is true and
parseLiteralOverridden is false, it also removes the inherited
coerceInputLiteral configuration. Preserve coerceInputLiteral when the resolver
explicitly overrides literal coercion, using the existing detection symbols in
the surrounding scalar setup.
🪄 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: 56f44631-bb26-48ef-ade2-2b0b1a78f5df
📒 Files selected for processing (24)
.changeset/bold-webs-behave.mdpackage.jsonpackages/executor/package.jsonpackages/executor/src/execution/__tests__/executor-test.tspackages/executor/src/execution/__tests__/oneOf-test.tspackages/executor/src/execution/__tests__/variables-test.tspackages/executor/src/execution/execute.tspackages/executor/src/execution/values.tspackages/executors/apollo-link/package.jsonpackages/executors/urql-exchange/package.jsonpackages/loaders/git/tests/loader.spec.tspackages/optimize/tests/remove-empty-nodes.spec.tspackages/optimize/tests/remove-loc.spec.tspackages/schema/src/addResolversToSchema.tspackages/schema/tests/schemaGenerator.test.tspackages/utils/src/collectFields.tspackages/utils/src/getArgumentValues.tspackages/utils/src/getDirectiveExtensions.tspackages/utils/src/graphqlJSCompat.tspackages/utils/src/index.tspackages/utils/tests/getArgumentValues.spec.tspackages/utils/tests/mapSchema.test.tspatches/@types+relay-compiler+8.0.4.patchpatches/subscriptions-transport-ws+0.11.0.patch
enisdenjo
left a comment
enisdenjo
left a comment
There was a problem hiding this comment.
- please update the CI to test v17 v16 and v15 https://github.com/ardatan/graphql-tools/blob/master/.github/workflows/tests.yml
| '@graphql-tools/utils': minor | ||
| --- | ||
|
|
||
| Add support for graphql-js v17 |
There was a problem hiding this comment.
since this PR is not only about bumping the version, it would be great to summarise what you did in order to get the v17 working. nothing too fancy, a summary of which issues were addressed
2 participants
Description
Adds support for graphql-js v17 while continuing to support v15 and v16 (the existing peer range).
Replaced the stale 4-bullet description with the full, accurate list (all the fixes we discussed were missing: the SDL-default-value bug, graphqlJSCompat.ts, the stale-closure fix, the two patches, CI changes).
Filled in "How Has This Been Tested?" with the real verification you actually did (typecheck/lint/prettier/build/full jest across v15/v16/v17).
Checked off boxes that are now genuinely true (no new warnings, tests added, tests pass locally) and left "documentation update" / "downstream modules" unchecked since those are still open.
Kept a note about Yoga, but reframed it from an in-progress status line into a clear scope caveat: executor-yoga's peer range is intentionally untouched, and that work is separate/not part of this PR — so a reviewer doesn't read "currently patching" as something still owed inside this PR.
Added a short "Further comments" note that this went through review during development and several bugs were caught and fixed inline rather than deferred.
Related # (issue)
Type of change
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Verified locally against graphql-js 15.10.2, 16.14.0/16.14.2, and 17.0.2 (pinning each via
scripts/match-graphql.jsand resetting the lockfile), running:tsc --noEmit,npm run lint,npm run prettier:check,npm run build && npx bob check, and the fulljestsuite. All passcleanly on all three versions; the only failures are pre-existing, sandbox-only Puppeteer/Chrome
download failures unrelated to this change.
Test Environment:
@graphql-tools/...: all packages in this monorepoChecklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
This PR was reviewed with an automated multi-pass code review during development; several
bugs it surfaced (a
sourcesmap that silently broke nested-variable resolution for customscalars implementing
coerceInputLiteral, a stale-closure bug in scalar rebuilding, and theSDL-default-value bug in
getArgumentValues) were found and fixed as part of this PR ratherthan left for follow-up.