Account for query's offset in file for errors #949
Conversation
src/language/__tests__/lexer-test.js
Outdated
| ' ?\n' + | ||
| '\n'; | ||
| const source = new Source(str, 'foo.js', { line: 11, column: 0 }); | ||
| return createLexer(source).advance() |
There was a problem hiding this comment.
Linter wants a semi here: https://travis-ci.org/graphql/graphql-js/jobs/253451244
src/language/source.js
Outdated
| this.body = body; | ||
| this.name = name || 'GraphQL request'; | ||
| this.location = location || { line: 1, column: 0 }; |
There was a problem hiding this comment.
Should column be 1-indexed here as well?
There was a problem hiding this comment.
For some reason lines are 1-indexed and columns are 0-indexed. I just looked at what Emacs prints in the modeline when I wrote this, but confirmed with the Babylon docs just now: https://github.com/babel/babylon/blob/master/ast/spec.md#node-objects
There was a problem hiding this comment.
Wow. That really makes no sense at all to me. In Vim, Atom, VSCode, TextMate, both line and column are 1-indexed. gcc also reports errors using 1-indexed lines and columns, and I suspect many other source-processing tools will as well.
src/error/syntaxError.js
Outdated
| @@ -22,9 +22,11 @@ export function syntaxError( | |||
| description: string | |||
| ): GraphQLError { | |||
| const location = getLocation(source, position); | |||
| const line = location.line + source.location.line - 1; | |||
There was a problem hiding this comment.
I suspect we don't want to name this source.location, to avoid confusion with the existing SourceLocation type and getLocation function. Maybe we could call it something like base or offset or something else.
There was a problem hiding this comment.
We should probably also handle the column offset too, but only on the first line. eg. if my source looks like this (ie. starting line 2, starting column 24):
12345678901234567890123456789
1:
2: graphql` ... X ...
3: Y ...
4:`
If there is an error at "X", we should report that as being on line 2, column 29, and if there is an error at "Y" then we should report it as being on line 3, column 3. Does that make sense?
There was a problem hiding this comment.
How about locationOffset?
Oh right! I meant to mention that in the pull request body. I wrote code to adjust for the column offset, but deleted it because it didn't seem worth the hassle for a pretty rare (as far as I can tell) special case. I can add the code again if you think it's important.
There was a problem hiding this comment.
locationOffset sounds pretty good.
src/language/__tests__/lexer-test.js
Outdated
| '\n' + | ||
| ' ?\n' + | ||
| '\n'; | ||
| expect(() => lexOne(str) |
There was a problem hiding this comment.
Might be worth mentioning in the commit message why you made this change.
There was a problem hiding this comment.
I'll just put it back :)
Adds support for providing the location in a file that the graphQL source was located. If the location is provided as part of the source, the line numbers in the formatted errors will be updated to account for the offset.
For example, in file Foo.js:
The lexer will throw an error with message:
It's up to the caller to provide the correct offset in the source.
Not sure if
locationis the best name for the field in theSourceobject. Very open to suggestions!