WIP: feat: field and value def autocomplete #1758

stonexer · 2021-01-10T07:34:30Z

New Functionality

This PR introduces "show all completion values" for an empty input for input types and object types. Before this, completion only appeared on the first typed character.

Once a user types fieldName: ▍ they should see all available values, including custom scalars defined in the schema, input types and inline defined input types

Testing

Check out either the graphiql 1 or monaco examples below, and try some basic SDL cases to explore the new functionality in the above features.

stonexer · 2021-01-10T07:36:04Z

packages/graphql-language-service-interface/src/__tests__/getAutocompleteSuggestions-test.ts

+      { label: '__DirectiveLocation' },
+      { label: '__TypeKind' },
+      { label: 'Episode' },
+      { label: 'InputType' },


This is probably an existing bug, as the InputType should probably not appear here

there's probably some simple logic we could add here for that if you want

to access the GraphQLObjectType in this case, I think you want state.prevState.type. you should be able to getFields() and then match the fieldname up to check the type

Thanks! But I don't seem to understand the role of field name here.

In my current thinking, since state.prevState.type(type Book) is a GraphQLObjectType, and InputType is a GraphQLInputObjectType, so it should not appear in the hintList.

my bad, misunderstood you before. i understand this bug better now. i’ll circle back after some sleep

codecov · 2021-01-10T07:38:14Z

Codecov Report

Merging #1758 (0f33d7a) into main (2d91916) will increase coverage by 0.19%.
The diff coverage is 68.62%.

@@            Coverage Diff             @@
##             main    #1758      +/-   ##
==========================================
+ Coverage   65.70%   65.90%   +0.19%     
==========================================
  Files          85       86       +1     
  Lines        5106     5144      +38     
  Branches     1631     1640       +9     
==========================================
+ Hits         3355     3390      +35     
- Misses       1747     1750       +3     
  Partials        4        4

Impacted Files	Coverage Δ
...ackages/graphiql-toolkit/src/create-fetcher/lib.ts	`50.90% <20.00%> (-8.67%)`	⬇️
packages/graphiql/src/utility/HistoryStore.ts	`62.26% <62.26%> (ø)`
packages/graphiql/src/components/QueryHistory.tsx	`73.91% <76.47%> (+6.69%)`	⬆️
...iql/src/components/DocExplorer/MarkdownContent.tsx	`100.00% <100.00%> (ø)`
packages/graphiql/src/components/GraphiQL.tsx	`58.34% <100.00%> (+0.83%)`	⬆️
packages/graphiql/src/components/QueryEditor.tsx	`63.96% <100.00%> (ø)`
...ervice-interface/src/getAutocompleteSuggestions.ts	`78.45% <100.00%> (+0.44%)`	⬆️
...hql-language-service-server/src/findGraphQLTags.ts	`64.89% <100.00%> (+4.25%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c03d4...0f33d7a. Read the comment docs.

stonexer · 2021-01-10T07:39:17Z

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

@@ -246,6 +248,16 @@ export function getAutocompleteSuggestions(
    return getSuggestionsForVariableDefinition(token, schema, kind);
  }

+  // Input value definition
+  if (kind === RuleKinds.INPUT_VALUE_DEF && step === 2) {


I'm not particularly sure what step means here, but I just found it to be representative of the current state. Hope to get some advice :)

yep, that's it! it represents the step in the parser rule you're at. some parser Rules have only one step, others have two.

here's where the magic happens in parser:

graphiql/packages/graphql-language-service-parser/src/onlineParser.ts

Line 256 in c4cba85

this is probably the most straightforward implementation example. with ARGUMENT, step 1 is when we want argument name, and step 2 is when we want an input value.

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

Line 161 in c4cba85

// Input values: Enum and Boolean

so, if i wanted to implement autocompletion for field names for object types that extended interfaces, i would do that on OBJECT_TYPE step 1.

oh and i guess you found one that has three steps! :D

Just thinking aloud.. is it possible for the steps to have constants for referencing them? In order to give the steps meaning beyond just 2 for example? 🤔

good question! they have a different meaning in the context of each rule. we could have Step.First but that wouldn't be much more readable, or would it?

Not really, it would pretty much be the same thing 😄 I'll think about it a bit more.

we could also specify enums for each rule that has multiple steps that are referenced, such as ObjectTypeSteps.Name = 1 and ObjectTypeSteps.Field = 2, and then FragmentDefintionSteps.Name = 1, etc

Maybe that would make sense, since the meaning of each step is defined by the GraphQL type being worked on. My only concern is that, since the values overlap, there might be an issue where they are mixed up. Typescript wouldn't complain since they are all numbers (except they are typed with the right enum type 🤔 ) but the behavior wouldn't be the expected behavior.

For example, InputObjectTypeSteps.Field might be equal to RuleKindStep.Name. Maybe my concern is a non-issue though. Perhaps if there is a way to type guard step to one of the enums, that would make step have just one meaning and remove ambiguity.

acao · 2021-01-10T09:03:40Z

awesome work so far! really appreciate your help as always. works great for me with object type fields and input type fields.

would you like me to look into a way to make it so that the autocomplete can begin a space after the fieldName:, rather than immediately? we have the same issues where i improved this for argument. or while you're in there, feel free!

stonexer · 2021-01-10T11:19:54Z

would you like me to look into a way to make it so that the autocomplete can begin a space after the fieldName:, rather than immediately?

Sure, that's what I want too.

I noticed that in the typescript file, when I type ...

interface Author {
  name: string;
}

interface Book {
  author:  ...
}

I don't get an active suggestion after I type : or : with a space. But then if I trigger ctrl+space manually, I can also have the full suggestion.

The difference is that when I type the first letter here, like A, there is an active suggestion directly.

I think it would be nice to have a similar logic here.

acao · 2021-01-11T20:17:14Z

@stonexer that's so odd, because it works in graphiql and monaco-graphql:

when you say a typescript file, are you referring to vscode-graphql?

stonexer · 2021-01-11T23:54:25Z

@acao Haha，it's not a bug here. Actually I mean the experience of using monaco to edit typescript files.

imolorhe

LGTM. Only concern I see would be the dropped test coverage. @acao is there a requirement for PRs not to reduce the test coverage?

acao · 2021-01-14T21:19:58Z

@imolorhe that is definitely something to consider! I disabled that once because a PR dropped coverage because of a refactor (I deleted so much redundant code with good coverage that it caused overall coverage to go down). I evaluate it on a case-by-case basis as a maintainer, but for this PR I think we should have additive coverage, though we're adding three cases.

I don't think this PR is ready myself because of the issue with spaces @stonexer and I discussed in discord, unfortunately we didn't surface that discussion here so let me summarize:

desired behavior for this PR:

autocompletion for variables should be triggered on : , non-breaking space included. ideally, the mode/ide implementation should be configured to advance the cursor the extra space automatically.

current behavior in this PR:
autocomplete behavior is triggered immediately following a : character, however we want there to be an additional space. if the user adds a space after the : character, then the completion options vanish. this is essentially the opposite behaviour.

imolorhe · 2021-01-14T21:24:14Z

Okay got it. Will un-approve and wait for that fix then 😄

stonexer · 2021-01-15T05:30:09Z

autocompletion for variables should be triggered on : , non-breaking space included. ideally, the mode/ide implementation should be configured to advance the cursor the extra space automatically.

In https://github.com/graphql/graphiql/blob/main/packages/monaco-graphql/src/languageFeatures.ts#L196

Are there other situations where we need ':' to trigger? Can we simply remove the ':' from them

acao · 2021-01-15T17:40:57Z

good question! we can experiment with that

stonexer · 2021-01-17T13:04:15Z

good question! we can experiment with that

Tried it, I think it's not bad

acao · 2021-01-17T13:59:20Z

this adjustment looks better for monaco editor, but it still doesn't fix the issue on the codemirror side. i wonder why? gonna keep looking into this!

acao · 2021-02-10T03:36:26Z

so sad i didn't merge this sooner! this is incredible

acao · 2021-02-10T03:39:36Z

@stonexer this is sooo close - maybe we can configure codemirror-graphql to handle the insertText param ourselves, since that's at this point only monaco specific?

acao · 2021-04-18T20:58:21Z

i would like to have this case covered, as I think it's one of the most important ones for the LSP. Many tools implement their own autocompletion hack on top of this to achieve it here

acao · 2021-04-18T21:25:13Z

@dwwoelfel this is another good place for the tokenizer

changeset-bot · 2021-09-03T09:52:25Z

⚠️ No Changeset found

Latest commit: 0f33d7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

linux-foundation-easycla · 2021-09-03T09:52:30Z

The committers are authorized under a signed CLA.

✅ SToneX (7075843, b91ba0f, b15f5f4, fef0417)

stonexer · 2021-09-04T04:24:56Z

@acao I'm back haha :), sorry for disappearing for a long time before for personal reasons. I added \s in AUTO_COMPLETE_AFTER_KEY, and suggestions are auto triggered on typing space. Could you help to have a review again?

acao · 2021-09-10T09:06:34Z

@stonexer so glad you are back! any help you can provide is always appreciated, no worries! definitely can review again soon, yes!

stonexer · 2021-10-26T09:49:49Z

Currently, the graphql schema in the context is derived from the parameters and does not contain the current input of the editor, so it looks like such a completion on types' field is not possible. A larger modification to onlineParser may be needed.

Referrence #888 (comment), I did some experimentations on graphql schema(sdl) editor. I made a dumb parser with chevrotain(a powerful tool), and just implemented a little bit of language service (adding support for type name and inputType name completion). Here's the demo and the source code.

Maybe someday we can implement this in a better way. 😃

acao · 2021-10-26T11:24:05Z

I will check this out locally and see how it’s working! This is such a valuable feature

acao · 2021-10-29T08:52:20Z

@stonexer note to test with the monaco graphql mode, just run yarn start-monaco at the monorepo root :)

acao · 2022-04-07T10:38:06Z

@stonexer I'd love to revive this effort soon! it's so close!

acao · 2022-04-07T10:45:11Z

@stonexer I've been looking at chevrotain's grammar for graphql since last year, and it's enticing. Incredibly fault tolerant.

I think it's worth looking at replacing the language parser with this entirely moving forward. It would mean re-writing autocomplete and hover entirely, but it would be worth it

stonexer commented Jan 10, 2021

View reviewed changes

stonexer changed the title ~~[WIP] feat: field and value def autocomplete~~ WIP: feat: field and value def autocomplete Jan 10, 2021

imolorhe approved these changes Jan 14, 2021

View reviewed changes

acao force-pushed the main branch 6 times, most recently from 4fa9ca3 to 1c11938 Compare February 1, 2021 18:22

acao added language-services enhancement labels Feb 10, 2021

stonexer added 3 commits September 4, 2021 12:07

add field def suggestions

b036455

add input value def suggestions

4101c87

feat(monaco): remove : in completion triggerCharacters

dc65988

stonexer force-pushed the feat/field-type-autocomplete branch from fef0417 to dc65988 Compare September 4, 2021 04:13

feat(graphiql): add space in codemirror autocomplete trigger keys

0f33d7a

acao force-pushed the main branch from f5265c7 to 7e98c6f Compare November 23, 2021 18:42

acao removed language-services labels Nov 12, 2022

WIP: feat: field and value def autocomplete #1758

Are you sure you want to change the base?

WIP: feat: field and value def autocomplete #1758

Conversation

stonexer commented Jan 10, 2021 • edited by acao Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

acao Jan 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Jan 10, 2021 • edited Loading

stonexer commented Jan 10, 2021

acao commented Jan 11, 2021 • edited Loading

stonexer commented Jan 11, 2021

imolorhe left a comment

Choose a reason for hiding this comment

acao commented Jan 14, 2021 • edited Loading

imolorhe commented Jan 14, 2021

stonexer commented Jan 15, 2021

acao commented Jan 15, 2021

stonexer commented Jan 17, 2021

acao commented Jan 17, 2021 • edited Loading

acao commented Feb 10, 2021

acao commented Feb 10, 2021

acao commented Apr 18, 2021

acao commented Apr 18, 2021

changeset-bot bot commented Sep 3, 2021 • edited Loading

⚠️ No Changeset found

linux-foundation-easycla bot commented Sep 3, 2021 • edited Loading

stonexer commented Sep 4, 2021 • edited Loading

acao commented Sep 10, 2021 • edited Loading

stonexer commented Oct 26, 2021 • edited Loading

acao commented Oct 26, 2021

acao commented Oct 29, 2021

acao commented Apr 7, 2022

acao commented Apr 7, 2022 • edited Loading

stonexer commented Jan 10, 2021 •

edited by acao

Loading

codecov bot commented Jan 10, 2021 •

edited

Loading

acao Jan 10, 2021 •

edited

Loading

acao commented Jan 10, 2021 •

edited

Loading

acao commented Jan 11, 2021 •

edited

Loading

acao commented Jan 14, 2021 •

edited

Loading

acao commented Jan 17, 2021 •

edited

Loading

changeset-bot bot commented Sep 3, 2021 •

edited

Loading

linux-foundation-easycla bot commented Sep 3, 2021 •

edited

Loading

stonexer commented Sep 4, 2021 •

edited

Loading

acao commented Sep 10, 2021 •

edited

Loading

stonexer commented Oct 26, 2021 •

edited

Loading

acao commented Apr 7, 2022 •

edited

Loading