Skip to content

Move maxTokens check to lexer#4668

Open
benjie wants to merge 3 commits into16.x.xfrom
lexer-max-tokens
Open

Move maxTokens check to lexer#4668
benjie wants to merge 3 commits into16.x.xfrom
lexer-max-tokens

Conversation

@benjie
Copy link
Copy Markdown
Member

@benjie benjie commented Apr 16, 2026

By checking maxTokens inside the createToken function we can be sure that it's applied consistently.

I've also moved ParserOptions.lexer to be an internal property (it should only be used by the schema coordinates parser, it was never intended to be a public interface) and implemented an assertion that forbids lexer and maxTokens together (since if they were both set, maxTokens would be ignored). I've also marked the SchemaCoordinateLexer as internal; we don't expose it via index.ts anyway, but best to signal the intent anyway.

I'm not super happy about mutating the internal state of the lexer from createToken - really I'd rather createToken were a method on the lexer class - but I don't want the two Lexers to have to implement it each and we don't like to extend classes so I think we can accept it in this one place. I'm not happy about it though, so if you have ideas for a better approach, please go ahead!

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
graphql-js Ignored Ignored Preview Apr 16, 2026 4:19pm

Request Review

@benjie benjie force-pushed the lexer-max-tokens branch from 87ee7b5 to 5aa6166 Compare April 16, 2026 16:19
@andimarek
Copy link
Copy Markdown
Contributor

You also want to limit the max chars overall + the max white space tokens (if they count differently)

Here are the limits GJ uses by default: https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/parser/ParserOptions.java

They should not be optional or off by default but we should rather rather set them to a high value 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