Skip to content

fix(graphql-language-service): allow optional field blocks in SDL type definitions#4329

Merged
trevor-scheer merged 3 commits into
graphql:mainfrom
vishwakt:fix/schema-kitchen-sink-spec-invalid
Jun 5, 2026
Merged

fix(graphql-language-service): allow optional field blocks in SDL type definitions#4329
trevor-scheer merged 3 commits into
graphql:mainfrom
vishwakt:fix/schema-kitchen-sink-spec-invalid

Conversation

@vishwakt
Copy link
Copy Markdown
Contributor

@vishwakt vishwakt commented Jun 4, 2026

Summary

The online parser used by codemirror-graphql required a field block on object, interface, input, and enum type definitions. Per the GraphQL spec those blocks are optional, so valid SDL like type Bar @baz or extend type Foo @onType (directives only, no body) was highlighted as an error (invalidchar).

This makes the field/value block optional, mirroring how SelectionSet is already optional on fields.

Closes #4206.

Changes

  • graphql-language-service: field/value blocks are now optional for object, interface, input, and enum type definitions (and their extensions) in the online parser grammar.
  • Corrected the schema-kitchen-sink.graphql test fixture, which contained spec-invalid empty bodies (extend type Foo @onType {} and type NoFields {}), to match graphql-js, and removed it from the oxfmt ignore list so it is formatted again.

How to verify

This affects the CodeMirror 5 graphql mode in codemirror-graphql. The GraphiQL netlify preview will not exercise it, since GraphiQL 5 uses Monaco rather than this parser.

To reproduce in a CodeMirror 5 editor:

import CodeMirror from 'codemirror';
import 'codemirror-graphql/esm/mode';

CodeMirror(document.getElementById('editor'), {
  value: 'type Bar @baz',
  mode: 'graphql',
});

On main, the type Bar @baz tokens render with the cm-invalidchar class (red). On this branch they render normally. The same applies to bodyless interface, enum, and input definitions and their extensions.

Before / after screenshots of that editor are attached in a comment below.

…e definitions

The online parser required a field/value block for object, interface, input,
and enum type definitions, but per the GraphQL spec these blocks are optional.
As a result spec-valid SDL such as `extend type Foo @onType` (directives only,
no body) was reported as invalidchar during highlighting.

Factor each brace block into its own rule and mark it optional, mirroring how
`SelectionSet` is already optional on fields. Also correct the
schema-kitchen-sink test fixture, which contained spec-invalid empty bodies
(`extend type Foo @ontype {}` and `type NoFields {}`), to match graphql-js, and
remove it from the oxfmt ignore list so it is formatted again.

Closes graphql#4206.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 4, 2026

🦋 Changeset detected

Latest commit: 4c1411f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
graphql-language-service Patch
codemirror-graphql Patch

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

@vishwakt
Copy link
Copy Markdown
Contributor Author

vishwakt commented Jun 4, 2026

Heads up for reviewers: the failing Types Check here is not introduced by this PR. It is a pre-existing latent error in graphql-language-service-server (a Logger import that only resolves through a fragile re-export chain), normally masked by Turborepo's cache. This PR changes graphql-language-service, which invalidates that cache and forces the real re-run, surfacing the error.

Full diagnosis and proposed fix: #4330 (fix in #4331)

This PR's own packages type-check and test cleanly; the red check will clear once #4331 lands and this branch is rebased.

Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing! I'll take a closer look at this at some point.

The entire verification section of your description is unhelpful to a reviewer. It would be much more helpful to provide a list of steps a reviewer can take to manually verify the fix. The netlify preview is usually a great place to start.

Fixes are always welcome, but please take the time to make your AI-generated descriptions easily digestible and coherent.

@vishwakt
Copy link
Copy Markdown
Contributor Author

vishwakt commented Jun 4, 2026

Before:
Screenshot 2026-06-04 at 10 25 31

After:
Screenshot 2026-06-04 at 10 26 29

@vishwakt
Copy link
Copy Markdown
Contributor Author

vishwakt commented Jun 4, 2026

@trevor-scheer thanks, that's fair. I've rewritten the description to drop the automated-results dump and added manual steps with before/after behavior.

Also added screenshots of the verification.

trevor-scheer pushed a commit that referenced this pull request Jun 5, 2026
…pc (#4331)

## Summary

`graphql-language-service-server` imported the `Logger` type from
`vscode-languageserver`, but `Logger` is defined in `vscode-jsonrpc` and
only reaches `vscode-languageserver` through a transitive re-export
(`export * from 'vscode-languageserver-protocol/'`). On CI this breaks
`types:check`:

```
src/GraphQLLanguageService.ts(56,15): error TS2305:
Module '"vscode-languageserver"' has no exported member 'Logger'.
```

This imports `Logger` from `vscode-jsonrpc` instead, which owns the type
and is already a direct dependency of the package (`vscode-jsonrpc:
^8.0.1`), avoiding the fragile re-export chain.

Fixes #4330.

## Two separate things are going on

These answer different questions and are both true:

**Why it surfaced now (Turborepo cache).**
`graphql-language-service-server#types:check` is a cache hit for any PR
that does not invalidate it, so the stale green result is reused. Only a
PR that changes `graphql-language-service` (a dependency) busts that
cache and forces a real re-run. That is when the latent error becomes
visible (it surfaced on #4329, a `graphql-language-service` change).

**Where the type error manifests (platform).** When `types:check`
actually runs, it fails on CI (Linux) but passes locally (macOS). The
lockfile pins identical dependency versions in both places
(`vscode-languageserver@8.0.1 ->
vscode-languageserver-protocol@3.17.1`), so resolution is the only
variable. `tsgo` is `@typescript/native-preview`, which ships a separate
compiled native binary per OS/arch (`native-preview-linux-x64` on CI vs
`native-preview-darwin-arm64` locally), and the pre-release binaries
appear to resolve the trailing-slash re-export differently.

I am confident about the cache behavior and the fix. The platform
mechanism is my best inference, since I can only test on macOS and
cannot directly inspect the Linux `tsgo`'s resolution. Either way the
fix is robust because it imports `Logger` from the package that owns it.

## Verification

- `tsgo --noEmit` and `oxlint` pass locally on the changed package.
- This PR's CI is the real verification: changing this file invalidates
the cached `types:check` and forces a fresh run on Linux, and `Types
Check` (along with all other checks) passes.
@trevor-scheer
Copy link
Copy Markdown
Contributor

Updating with merge after landing your fix, expecting type check to pass now.

Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix LGTM. It looks like the OnlineParser tests only update the existing tests? Can we add new tests for the cases we're allowing now (bare types/interfaces/etc with no selection set)?

…n online parser

Add explicit OnlineParser cases for object, interface, enum, and input type
definitions (and a type extension) that have no body, each followed by another
definition so the test exercises parser recovery. These fail against the
previous grammar and pass with the optional field block.
Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@trevor-scheer trevor-scheer merged commit a526a10 into graphql:main Jun 5, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

schema-kitchen-sink.graphql contains spec-invalid GraphQL

2 participants