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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

[lsp-server] 馃悶 Adding whitespaces\newlines causes autocompletion to move up a level #3553

Open
1 task done
rrousselGit opened this issue Mar 6, 2024 · 5 comments 路 May be fixed by #3554
Open
1 task done

[lsp-server] 馃悶 Adding whitespaces\newlines causes autocompletion to move up a level #3553

rrousselGit opened this issue Mar 6, 2024 · 5 comments 路 May be fixed by #3554
Labels
bug lsp-server graphql-language-service-server

Comments

@rrousselGit
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Consider the following code:

mutation createComment2 @auth(is: PUBLIC) {
  comment_insert2(data: {  
  
  })
}

where data expects an input type.

In this scenario, adding whitespaces when trying to get autocompletion seems to change the result. Here's a video showcasing the issue:

Screen.Recording.2024-03-06.at.13.06.38.mov

Expected Behavior

Adding whitespaces/newlines should not have any impact on the suggestion.

Steps To Reproduce

No response

Environment

  • LSP Server Version: latest
  • OS: macos
  • LSP Client: vscode-graphql

Anything else?

I've tried debugging this by adding logs on a fork of the repository, and the issue appears to be getAutocompletionSuggestions.ts#getTokenAtPosition.

I added a console.log(token) inside getAutocompleteSuggestions right before the call to getTypeInfo here

graphiql/packages/graphql-language-service/src/interface/getAutocompleteSuggestions.ts

Line 181 in ece99f6

const typeInfo = getTypeInfo(schema, token.state);

Then compared the log outputs based on where the cursor is.

Given comment_insert2(data: {| }), the token returned is:

{
  start: 0,
  end: 0,
  string: '{',
  state: { ... },
  style: 'punctuation'
}

But for comment_insert2(data: { | }) it is:

{
  start: 0,
  end: 0,
  string: '}',
  state: { ... },
  style: 'punctuation'
}

And lastly for comment_insert2(data: { |}) it is:

{
  start: 0,
  end: 0,
  string: ')',
  state: { ... },
  style: 'punctuation'
}

This doesn't make sense to me. I would expect all of them to return the token with string: '{'
@rrousselGit rrousselGit added bug lsp-server graphql-language-service-server labels Mar 6, 2024
@acao
Copy link
Member

acao commented Mar 6, 2024

there are some bugs I'm planning to fix in autocomplete that may be related to this, or it's an offset issue. Is the file you are editing named . graphql or .graphqls by chance, or something else?

@rrousselGit
Copy link
Author

Is the file you are editing named . graphql or .graphqls by chance, or something else?

No it is named .gql actually

@rrousselGit
Copy link
Author

I think CharacterStream#getCurrentPosition is wrong.

I've logged the cursor and stream.getCurrentPosition() in getTokenAtPosition, and the cursor matches what I expected. But the position of }/) is off.

Given the line:

  comment_insert2(data: {    }   )

} is said to be at column26 and ) at 27. But they are in fact at respectively 29 and 33

@rrousselGit
Copy link
Author

rrousselGit commented Mar 6, 2024 •
edited

Looks like disabling "eatWhitespace" fixes the whitespace issue (although likely an incorrect fix)

export function runOnlineParser(
  queryText: string,
  callback: callbackFnType,
): ContextToken {
  const lines = queryText.split('\n');
  const parser = onlineParser({
+     // Preserve whitespaces for the sake of realistic offsets.
+    eatWhitespace: () => false,
+    // Keep default values, as TS requires those fields to be specified
+    lexRules: LexRules,
+    parseRules: ParseRules,
+    editorConfig: {},
  });

Newlines are still a problem though. I'll look into it more

@rrousselGit
Copy link
Author

I think for newlines the issue is this condition here:

https://github.com/graphql/graphiql/blob/ece99f63f5d8d01057b735e90a6957edea3e42b9/packages/graphql-language-service/src/interface/getAutocompleteSuggestions.ts#L973C1-L974C1

In the case of:

  foo(data: {
    |

I assume we'd want to obtain { for the token under the cursor. But the cursor isn't in the same line as the token, so that condition is bound to fail I think.
My guess is, we should update the logic such that when the cursor is between two tokens, we pick the first one. At the moment, it'll always fail to pick anything

With both changes considered, that seem to fix the issue for me. I'll open a PR

rrousselGit added a commit to rrousselGit/graphiql that referenced this issue Mar 6, 2024
@rrousselGit
Fix autocomplete suggestion
5ec5b33 
fixes graphql#3553
@rrousselGit rrousselGit linked a pull request Mar 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug lsp-server graphql-language-service-server
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix autocomplete suggestion rrousselGit/graphiql
2 participants
@acao @rrousselGit