Skip to content
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

[language-service] getRange does not work for line 0 #3168

Open
ValtsS opened this issue May 9, 2023 · 6 comments
Open

[language-service] getRange does not work for line 0 #3168

ValtsS opened this issue May 9, 2023 · 6 comments

Comments

@ValtsS
Copy link

ValtsS commented May 9, 2023

Seems there is a bug here:

The call comes in from doHover(..) if its called on the 1st line - it comes in as line 0. So the for loop never triggers and an invariant error is thrown.

export function getRange(location: SourceLocation, queryText: string): IRange {
  const parser = onlineParser();
  const state = parser.startState();
  const lines = queryText.split('\n');

  invariant(
    lines.length >= location.line,
    'Query text must have more lines than where the error happened',
  );

  let stream = null;

  for (let i = 0; i < location.line; i++) { // Likely should be <=
    stream = new CharacterStream(lines[i]);
    while (!stream.eol()) {
      const style = parser.token(stream, state);
      if (style === 'invalidchar') {
        break;
      }
    }
  }

  invariant(stream, 'Expected Parser stream to be available.');

@acao
Copy link
Member

acao commented May 10, 2023

#3149 I believe will fix this?

@acao
Copy link
Member

acao commented May 10, 2023

would you say this is a duplicate of #2874?

@ValtsS
Copy link
Author

ValtsS commented May 10, 2023

Seems related, but I don't know the code well enough. But I am not sure it would, the call is coming from here

image

lands here
image

and we have line 0 while in fact its the 1st line. It might not be 1st token, just fact that it s on 1st line breaks it.

@ValtsS
Copy link
Author

ValtsS commented May 10, 2023

Also this part

  invariant(
    lines.length >= location.line,
    'Query text must have more lines than where the error happened',
  );

tells me that the issue is in this function, since invariant expects 0 based line index. Yet it does not work when there is just line 0:

for (let i = 0; i < location.line; i++) { 
...

@acao
Copy link
Member

acao commented May 10, 2023

hmm, indeed. if I understand correctly, i think this is why we had to offset lines in packages/monaco-graphql/src/LanguageService.ts, because if I recall, the LSP spec counts lines from 1, whereas monaco counts lines from 0

have you seen this surface as an issue in any specific implementation such as graphiql or monaco-graphql or the LSP server, or are you making a new implementation directly from getHover()?

@ValtsS
Copy link
Author

ValtsS commented May 10, 2023

I am using this with monaco

"monaco-editor": "^0.38.0",
"monaco-graphql": "^1.2.2",

As you see the actual query is just 1 liner (incomplete) = "query { }"
So the hover is coming from monaco. There is no custom implementation on my part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants