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

[relay-lsp] Suggestions from wrong context #4472

Open
tobias-tengler opened this issue Oct 7, 2023 · 4 comments
Open

[relay-lsp] Suggestions from wrong context #4472

tobias-tengler opened this issue Oct 7, 2023 · 4 comments

Comments

@tobias-tengler
Copy link
Contributor

tobias-tengler commented Oct 7, 2023

Last week, while working on a rather large file, I noticed that the LSP would often not suggest any fields or fields from a different level in the selection set.

The issue is easily reproduced (more so in larger files) if you accept the completion item for a field returning an object type.

In response to that the LSP will insert a snippet like the following:

newObjectField {
    $cursor
}

Afterwards it triggers the editor.action.triggerSuggest command to reopen the completion menu at the new cursor position.

What's happening is that the textdocument/didchange and textdocument/completion commands from the client are not necessarily sent in that order and if they are, the LSP might still be processing the document update while completion items are already being calculated at the new cursor position. And of course, if the LSP is computing completion items for the new location, while still working with the old document, weird things can happen like no suggetions because the position is outside the GraphQL document or the position being in a different selection set.

I think there are two things at play here that both need to be addressed in order to correct the behavior:

  1. Relay should wait to calculate completion items until all sync operations are completed - this can be easily done and I will send a PR for it in a follow-up. EDIT: [relay-lsp] Ensure documents are synced before calculating completions #4473
    2. There seems to be a bug in the LSP client (VS Code in this case) where textdocument/didchange and textdocument/completion are not sent in this order if a completion item triggers editor.action.triggerSuggest - I will raise an issue for this in their repository.
@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Oct 8, 2023

There actually doesn't appear to be an issue with the LSP client (VSCode). textdocument/didchange and textdocument/completion actually arrive in the correct order at the server. They just aren't processed in the correct order:

image

I'm still investigating why...

@zth
Copy link
Contributor

zth commented Oct 10, 2023

This would be fantastic to fix indeed.

@tobias-tengler
Copy link
Contributor Author

Got another report from a team struggling with this today. Would be awesome if my fix in #4473 could be considered to be merged.
Honestly surprised this isn't a problem at Meta or other big companies...

@captbaritone
Copy link
Contributor

I'm out this week but Cc @tyao1

facebook-github-bot pushed a commit that referenced this issue Jul 11, 2024
Summary:
There's an infra issue with the Relay repo and our PR import code which meant the automated import (D58590396) failed.

 This is a manual import of #4472 needed due to an infra issue on Meta's side. Full credit to tobias-tengler for the contents of this commit.

Reviewed By: tyao1

Differential Revision: D59645988

fbshipit-source-id: ad252f0fa3229a9e45a8b779e4eb52ead7b5b7e8
monicatang pushed a commit to monicatang/relay that referenced this issue Aug 1, 2024


Summary:
There's an infra issue with the Relay repo and our PR import code which meant the automated import (D58590396) failed.

 This is a manual import of facebook#4472 needed due to an infra issue on Meta's side. Full credit to tobias-tengler for the contents of this commit.

Reviewed By: tyao1

Differential Revision: D59645988

fbshipit-source-id: ad252f0fa3229a9e45a8b779e4eb52ead7b5b7e8
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 a pull request may close this issue.

3 participants