Skip to content

Schema coordinates #4463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: schema-coordinates
Choose a base branch
from

Conversation

magicmark
Copy link

@magicmark magicmark commented Jul 27, 2025

@benjie @leebyron This PR implements the changes on top of the existing schema-coordinates branch as discussed in July's WG meeting:

https://github.com/graphql/graphql-wg/blob/main/notes/2025/2025-07.md#schema-coordinates

Specifically:

  • introduces a new SchemaCoordinateLexer class inside src/language/schemaCoordinateLexer
  • imports and reuses the existing {createToken, printCodePointAt, readName} helpers
  • creates a new LexerInterface interface to allow the helpers to accept either instance of a Lexer without copy/pasting.

What I dislike about this PR:

  • continues to "infect" the rest of parser.ts with the knowledge/existence of schema coordinates (LexerInterface)
  • does not meet the "this is it's own thing and tree-shakable" dream as discussed in the meeting

What I like about this PR:

  • seems like the minimal diff required to do this "properly" (including locations, useful error messages)

tl;dr if we tried to completely devolve Parser of responsibility of schema coordinates, and started on a minimal implementation of parseSchemaCoordinate() that advances a lexer instance token by token, we'd end up at something very similar to Parser.... I did try a few versions of this and copy and pasting things but this ended up being neater overall.

FWIW I did start to a attempt a minimal parseSchemaCoordinate() function here: https://github.com/magicmark/graphql-js/blob/586b7174dc730fef47368ce4fbc385fb1f733360/src/language/schemaCoordinate.ts#L44

but it turns out that doing it imperatively and correctly without a nice lexer/parser is kinda hard! the more logic I add to this, the closer I get to essentially recreating a lexer/parser... which we already have :)

wdyt? should we keep trying for more of a middle ground here?

@magicmark magicmark marked this pull request as ready for review July 27, 2025 06:38
@magicmark magicmark requested a review from a team as a code owner July 27, 2025 06:38
@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jul 28, 2025

does not meet the "this is it's own thing and tree-shakable" dream as discussed in the meeting

My two cents on this is that we already have quite a bloated idea of a parser/lexer, in an ideal world we would split up the parser to be the Definition- and Execution Language. Which would help clients tree-shake down to a minimal visit, print and parse implementation rather than what we have now.

Example:

This mostly to say that to push a spec forward I wouldn't really be bothered too much about the increase in bundle-size/lack of tree-shaking. For clients we are already massively overweight as we deliver SDL regardless of your needs.

In the ideal world scenario the coordinates parser could be tied to the SDL lexer which would exempt clients from importing this by default.

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.

2 participants