Feature: LSP Fragment completions #4134

beaumontjonathan · 2022-11-21T14:47:01Z

LSP Fragment Completions

Adds support for __typename, fragment, and inline fragment completions for interface/union/type.

Completions are ordered using sortText, with the following precedence:

Fields on the type, e.g., fields from type/interface
__typename
...on T inline fragments
...F fragments

The modified tests show the behaviour, but a summary here is:

`interface`

__typename: String! completion
...on T completions for the interface type itself and all types which implement it
...F completions for all fragments F on the interface or any implements' of it

Example

`union`

__typename: String! completion
...on T completions for the union type itself and for each variant
...F completions for all fragments F on the union or any variants of it

Example

`type`

__typename: String! completion
...F completions for all fragments F on the type or on any interfaces or unions it implements

Example

Questions:

Does Relay have a preferred style for spacing of fragments? Which should be used on these completions? Should this be a user configurable option?
- ...on T vs ... on T
- ...F vs ... F
Should inline fragment completions exist for the type itself? E.g., when fragmenting on a type User, should ...on User be suggested?
- At the moment this MR adds this for interface & union but not type - should this be consistent?
Will this feature be annoying for users of the LSP in orgs with a large codebase & lots of fragments? Should either of these be user configurable?
Is the sortText robust enough? This has been implemented by prefixing a number to the start of the label; it seems to work for me locally (code)

… `Union` types

alunyov · 2022-11-21T15:13:19Z

compiler/crates/relay-lsp/src/completion/mod.rs

@@ -710,6 +753,7 @@ fn resolve_completion_items_for_inline_fragment_type(
    type_: Type,
    schema: &SDLSchema,
    existing_inline_fragment: bool,
+    include_fragment: bool,


nit: boolean arguments maybe confusing sometimes.

Maybe we should change this into the callback -> format_fragment_completion_label?

I completely agree about boolean arguments, I rewrote with a callback and it seemed more confusing

We already use a boolean argument in a similar situation for resolve_completion_items_for_fragment_spread. Here existing_fragment_spread: bool is used to describe whether the completion is being used to rename an existing spread.

I've renamed this function and argument for inline fragments to more closely align to the existing one for fragment spreads. Now the argument is existing_fragment_spread.

alunyov

This is super cool!

Thank you for working on this, just one small comment on the boolean argument, but overall looks great.

alunyov · 2022-11-21T15:21:11Z

Should inline fragment completions exist for the type itself? E.g., when fragmenting on a type User, should ...on User be suggested?

I don't think we need this, I think we automatically inline these fragments anyways, so they are redundant.

Will this feature be annoying for users of the LSP in orgs with a large codebase & lots of fragments? Should either of these be user configurable?

I think it should be fine. As long as you can refine the list of suggestions base on what you've already typed. Can you? Will it filter out irrelevant fragment spreads?

Is the sortText robust enough?

Hmm, seems like it should be fine.

beaumontjonathan · 2022-11-21T19:20:50Z

Awesome, thanks for looking at this so quickly @alunyov! :D

Should inline fragment completions exist for the type itself? E.g., when fragmenting on a type User, should ...on User be suggested?

I don't think we need this, I think we automatically inline these fragments anyways, so they are redundant.

Cool, I can make a small change to filter out the base type for interface & union so it has the same behaviour as type, i.e., so the following doesn't happen

fragment Test on MyInterfaceType {
  |
  # Completions:
  # ...on MyInterfaceType     <-- Remove this one
  # ...on ConcreteType
}

Will this feature be annoying for users of the LSP in orgs with a large codebase & lots of fragments? Should either of these be user configurable?

I think it should be fine. As long as you can refine the list of suggestions base on what you've already typed. Can you? Will it filter out irrelevant fragment spreads?

This is the behaviour while typing - so long as it exactly matches the completion label then it persists, but if you close the completion window & reopen it, then you don't get any completions (*except for when you're writing a fragment spread, i.e., ...Foo| because that's handled explicitly here)

This logic could definitely be improved, but it would mean adjusting the AST to allow inline fragments with no identifiers, i.e., allowing ...on |.

Screen.Recording.2022-11-21.at.19.06.04.mov

… inline fragment

…g_fragment_spread`

beaumontjonathan · 2022-11-21T22:01:13Z

@alunyov I've made some changes to clean up the arguments a little, and in the process explored how completion works for existing inline fragments. I added more tests to document this & fixed the behaviour in my change.

There is still plenty of weirdness in relay-lsp, but I don't think any of these changes degrades it. There's definitely more I'd like to explore about allowing completion items which aren't just at the start of the line, e.g., a partially typed inline fragment which currently doesn't work.

{
  ...on StartOfFragmentNam|
  # ^ Completions
  # - ...on StartOfFragmentName
  # - ...on StartOfFragmentNameOther
}

Cool, I can make a small change to filter out the base type for interface & union so it has the same behaviour as type, i.e., so the following doesn't happen

Also I checked this out again, and at the moment this MR behaves the same way as GraphiQL, so I'm tempted to keep it as is.

Let me know if you'd like anything else changed 😄

facebook-github-bot · 2022-11-22T13:54:22Z

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

alunyov · 2022-11-22T15:15:24Z

Thank you @beaumontjonathan for working on this. This looks great!

I'll test this internally, and I think we can ship this.

A few ideas on how we can improve this further (but maybe as a follow-up PRs);

I think currently, if you type ... it doesn't show the list of fragments. I think it wasn't working before this PR, but since we start showing fragment spreads in the suggestion list, it would be cool if we can start showing all possible fragment spreads in the case when people type ....
Apply lexicographical sorting to the suggestion list. We use this for error suggestions here:

relay/compiler/crates/schema/src/suggestion_list.rs

Line 65 in 908fe11

let lexical_distance = LexicalDistance::new(input.lookup());

)
- for field names: use the user input and show suggestions in the order that is closed in this lexicographical distance to already typed value.
- for fragment spreads: use the current operation/fragment name as the sorting reference for the suggestion list. It is a heuristic, but you more likely to use similar fragments (something like UserProfile, UserProfilePicture, UserProfileDescription, etc... are more likely the correct fragment to suggest first, rather then something that's starts with "A" like ...ApplicationSettings, etc).

compiler/crates/relay-lsp/src/completion/mod.rs

beaumontjonathan · 2022-11-22T21:47:11Z

I think currently, if you type ... it doesn't show the list of fragments. I think it wasn't working before this PR, but since we start showing fragment spreads in the suggestion list, it would be cool if we can start showing all possible fragment spreads in the case when people type ....

This is reasonable, and definitely the desired behaviour.

I looked into this initially, but it would require a change to the parser - at the moment we won't recognize that it's a fragment unless if has a identifier after the ... code.

It's definitely possible to fix this, but would have a larger blast radius & I think it could come later.

Apply lexicographical sorting to the suggestion list. We use this for error suggestions here:

relay/compiler/crates/schema/src/suggestion_list.rs

Line 65 in 908fe11

let lexical_distance = LexicalDistance::new(input.lookup());

^ This is really interesting. For editor completions, I think if we get the above working, then we should get this for free from the editor. This is the current behaviour for field completions in VSCode

Screen.Recording.2022-11-22.at.21.35.43.mov

facebook-github-bot · 2022-11-22T23:25:43Z

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

alunyov · 2022-11-23T14:57:54Z

Oh, one valid case that we need to fix here. We should not add __typename for the root types (Query, Mutation, Subscription).

We have a validation that prevents from adding these fields:

relay/compiler/crates/relay-transforms/src/validations/disallow_typename_on_root.rs

Lines 45 to 47 in 15cc327

    
           Err(vec![Diagnostic::error( 
        
               ValidationMessage::DisallowTypenameOnRoot, 
        
               typename_selection.definition.location,

So we should not add this to the autocomplete list for these types.

… for this

facebook-github-bot · 2022-11-23T21:15:37Z

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-11-29T03:30:09Z

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: # LSP Fragment Completions Adds support for `__typename`, fragment, and inline fragment completions for `interface`/`union`/`type`. Completions are ordered using [`sortText`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion), with the following `precedence`: 1. Fields on the type, e.g., fields from `type`/`interface` 2. `__typename` 3. `...on T` inline fragments 4. `...F` fragments The modified tests show the behaviour, but a summary here is: ## `interface` - `__typename: String!` completion - `...on T` completions for the `interface` type itself and all types which implement it - `...F` completions for all fragments `F` on the `interface` or any `implements`' of it ### Example <img width="468" alt="Screenshot 2022-11-21 at 13 59 39" src="https://user-images.githubusercontent.com/17055343/203078448-465eb2ab-6165-49ee-9f9d-403bcbe6f739.png"> ## `union` - `__typename: String!` completion - `...on T` completions for the `union` type itself and for each variant - `...F` completions for all fragments `F` on the `union` or any variants of it ### Example <img width="471" alt="Screenshot 2022-11-21 at 13 59 56" src="https://user-images.githubusercontent.com/17055343/203078444-9f98c439-facb-4c5d-ab0b-ccf552fca0e2.png"> ## `type` - `__typename: String!` completion - `...F` completions for all fragments `F` on the `type` or on any `interface`s or `union`s it implements ### Example <img width="469" alt="Screenshot 2022-11-21 at 14 00 10" src="https://user-images.githubusercontent.com/17055343/203078437-10b43e4d-152c-4592-a7e9-2bfad020cde4.png"> ## Questions: - [x] Does Relay have a preferred style for spacing of fragments? Which should be used on these completions? Should this be a user configurable option? - `...on T` vs `... on T` - `...F` vs `... F` - [x] Should inline fragment completions exist for the type itself? E.g., when fragmenting on a `type User`, should `...on User` be suggested? - At the moment this MR adds this for `interface` & `union` but not `type` - should this be consistent? - [x] Will this feature be annoying for users of the LSP in orgs with a large codebase & lots of fragments? Should either of these be user configurable? - [x] Is the `sortText` robust enough? This has been implemented by prefixing a number to the start of the `label`; it seems to work for me locally ([code](https://github.com/facebook/relay/pull/4134/files#diff-04b7155ac238238b6434c2f32c30439229c0127075572f2d2f7519b574ff90c6R1018)) Pull Request resolved: facebook#4134 Reviewed By: tyao1 Differential Revision: D41470372 Pulled By: alunyov fbshipit-source-id: ef04a34fb7e145cb9884c8b79345fd8e0a26cf83

beaumontjonathan added 3 commits November 20, 2022 14:26

Added fragment & __typename completions for Interface, Object &…

e560462

… `Union` types

Added tests

7ed3cea

Fmt

693cc37

facebook-github-bot added the CLA Signed label Nov 21, 2022

alunyov reviewed Nov 21, 2022

View reviewed changes

alunyov requested changes Nov 21, 2022

View reviewed changes

beaumontjonathan added 2 commits November 21, 2022 21:26

Fix & add tests for edge case where an inline fragment already has an…

3154e48

… inline fragment

Rename & negate boolean parameter from include_fragment to `existin…

029fef5

…g_fragment_spread`

Merge branch 'main' into jb_improved-fragments

287255e

alunyov approved these changes Nov 22, 2022

View reviewed changes

alunyov reviewed Nov 22, 2022

View reviewed changes

compiler/crates/relay-lsp/src/completion/mod.rs Outdated Show resolved Hide resolved

beaumontjonathan added 2 commits November 22, 2022 21:19

Merge branch 'main' into jb_improved-fragments

7e9f643

Add space before on for inline fragment completions

95eb846

beaumontjonathan added 3 commits November 23, 2022 19:15

Remove __typename completion suggestion on root fields; added tests…

ef8a4e6

… for this

Fmt

04e71b7

Merge branch 'main' into jb_improved-fragments

f6305de

Merge branch 'main' into jb_improved-fragments

90929ff

facebook-github-bot closed this in 96c7193 Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: LSP Fragment completions #4134

Feature: LSP Fragment completions #4134

beaumontjonathan commented Nov 21, 2022 •

edited

Loading

alunyov Nov 21, 2022

beaumontjonathan Nov 21, 2022

alunyov left a comment

alunyov commented Nov 21, 2022

beaumontjonathan commented Nov 21, 2022 •

edited

Loading

beaumontjonathan commented Nov 21, 2022 •

edited

Loading

facebook-github-bot commented Nov 22, 2022

alunyov commented Nov 22, 2022

beaumontjonathan commented Nov 22, 2022

facebook-github-bot commented Nov 22, 2022

alunyov commented Nov 23, 2022

facebook-github-bot commented Nov 23, 2022

facebook-github-bot commented Nov 29, 2022

Feature: LSP Fragment completions #4134

Feature: LSP Fragment completions #4134

Conversation

beaumontjonathan commented Nov 21, 2022 • edited Loading

LSP Fragment Completions

interface

Example

union

Example

type

Example

Questions:

alunyov Nov 21, 2022

Choose a reason for hiding this comment

beaumontjonathan Nov 21, 2022

Choose a reason for hiding this comment

alunyov left a comment

Choose a reason for hiding this comment

alunyov commented Nov 21, 2022

beaumontjonathan commented Nov 21, 2022 • edited Loading

beaumontjonathan commented Nov 21, 2022 • edited Loading

facebook-github-bot commented Nov 22, 2022

alunyov commented Nov 22, 2022

beaumontjonathan commented Nov 22, 2022

facebook-github-bot commented Nov 22, 2022

alunyov commented Nov 23, 2022

facebook-github-bot commented Nov 23, 2022

facebook-github-bot commented Nov 29, 2022

beaumontjonathan commented Nov 21, 2022 •

edited

Loading

`interface`

`union`

`type`

beaumontjonathan commented Nov 21, 2022 •

edited

Loading

beaumontjonathan commented Nov 21, 2022 •

edited

Loading