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

feat: interfaces implementing interfaces for the LSP #1742

Merged
merged 3 commits into from
Dec 28, 2020

Conversation

acao
Copy link
Member

@acao acao commented Dec 22, 2020

Features

This introduces a few new language features for implements syntax, including the new(ish) & operator, both for interfaces & types.

Parsing & Highlighting

This implements support for & in the parser, which means highlighting support for codemirror graphql

Autocompletion

Previously there was no autocompletion for any of this

type Interface implements So

and

type Interface implements Something & So

monaco-autocomplete-interfaces

now we have

Inline Autocompletion

it will also autocomplete for inline interfaces that may not be in the schema yet, for writing SDL files for example.

interface Example { id: String }
type Interface implements Ex

Where this feature works

  • parser & interface
  • graphql-language-service-server
  • vscode-graphql
  • monaco-graphql
  • codemirror-graphql highlighting for & syntax works because of the parser implementation in this PR

HTD

(netlify)

  1. open the monaco netlify demo linked in CI jobs below
  2. provide github personal access token to demo (no scopes needed, delete when finished from localstorage! oauth demo coming soon)
  3. begin building types and interfaces that implement the github interfaces and eachother
  4. confirm autocompletion works everywhere between the keyword “implements” and the first directive or { bracket

(manual)

  1. clone repo
  2. yarn
  3. yarn build
  4. yarn start-monaco
  5. follow from step 2 above to the end

To see how this will impact graphiql, run yarn start-graphiql, and you’ll see the highlighting/parsing work for & syntax now.

What's Next

  • codemirror-graphql has an additional step for implementing these new autocompletion features. @yoshiakis and I found that codemirror-graphql’s hint.js is mostly a dupe of getAutocompleteSuggestions. So in the next PR, we will be able to replace most of whats in hint.js for codemirror with getAutocompleteSuggestions. This will make feature parity between monaco-graphql and codemirror-graphql much easier to maintain
  • autocompletion for interface fields when extending an interface
  • consider adding more validation rules for interfaces or object types (spec decision)

@acao acao marked this pull request as draft December 22, 2020 21:40
@acao acao force-pushed the feat/interfaces-interfaces branch 2 times, most recently from f1a4751 to 82d0e18 Compare December 23, 2020 15:34
@acao acao marked this pull request as ready for review December 23, 2020 15:36
@codecov
Copy link

codecov bot commented Dec 23, 2020

Codecov Report

Merging #1742 (bf7982f) into main (d81db0c) will increase coverage by 0.18%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
+ Coverage   66.79%   66.97%   +0.18%     
==========================================
  Files          87       87              
  Lines        4873     4906      +33     
  Branches     1343     1351       +8     
==========================================
+ Hits         3255     3286      +31     
- Misses       1383     1385       +2     
  Partials      235      235              
Impacted Files Coverage Δ
...kages/graphql-language-service-parser/src/Rules.ts 97.80% <0.00%> (-2.20%) ⬇️
...kages/graphql-language-service-parser/src/types.ts 100.00% <ø> (ø)
...-service-parser/src/__tests__/OnlineParserUtils.ts 97.59% <85.71%> (-2.41%) ⬇️
...ervice-interface/src/getAutocompleteSuggestions.ts 74.10% <96.15%> (+2.04%) ⬆️
...hql-language-service-parser/src/CharacterStream.ts 100.00% <100.00%> (ø)
...raphql-language-service-parser/src/onlineParser.ts 94.89% <100.00%> (ø)
...anguage-service-interface/src/autocompleteUtils.ts 95.06% <0.00%> (+1.23%) ⬆️

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 d81db0c...bf7982f. Read the comment docs.

@@ -320,6 +324,56 @@ function getSuggestionsForInputValues(
return [];
}

function getSuggestionsForImplements(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanGoncharov here is the working implementation so far!

@acao
Copy link
Member Author

acao commented Dec 24, 2020

@yoshiakis so I've gotten this working in codemirror-graphql... by replacing hint.js almost entirely 🤯. only one failing test left, but that doesn't mean there won't be other regressions

I decided to do what we discussed a while ago, and made type be passed by getAutocompleteSuggestions in interface itself.

so far, just this is working for all but one test:

import {
  getAutocompleteSuggestions,
} from 'graphql-language-service-interface';

CodeMirror.registerHelper('hint', 'graphql', (editor, options) => {
  const schema = options.schema;
  if (!schema) {
    return;
  }

  const cur = editor.getCursor();
  const token = editor.getTokenAt(cur);
  const rawResults = getAutocompleteSuggestions(
    schema,
    editor.getValue(),
    token,
  );
  const tokenStart =
    token.type !== null && /"|\w/.test(token.string[0])
      ? token.start
      : token.end;
  const results = {
    list: rawResults.map(item => ({
      text: item.label,
      type: item.type ? String(item.type) : String(item.detail),
      description: item.documentation,
      isDeprecated: item.isDeprecated,
      deprecationReason: item.deprecationReason,
    })),
    from: { line: cur.line, column: tokenStart },
    to: { line: cur.line, column: token.end },
  };

  if (results && results.list && results.list.length > 0) {
    results.from = CodeMirror.Pos(results.from.line, results.from.column);
    results.to = CodeMirror.Pos(results.to.line, results.to.column);
    CodeMirror.signal(editor, 'hasCompletion', editor, results, token);
  }

  return results;
});

it seems this would significantly reduce the effort of introducing new language features to codemirror-graphql!

@yoshiakis
Copy link
Collaborator

I got it! It makes sense to do so. Though in that case, I think we should change this part so that graphql-language-service-server doesn't send information on type when is requested auto complete suggestions list in order to reduce the amount of data sent.

@yoshiakis
Copy link
Collaborator

@acao Why do you want to change the type of item.type to a string type?

@acao
Copy link
Member Author

acao commented Dec 24, 2020

@acao Why do you want to change the type of item.type to a string type?

this is what caused certain tests to pass, it seems to be that the tests expect type to be a string

@acao
Copy link
Member Author

acao commented Dec 25, 2020

@yoshiakis - Just to be clear, the only impact this PR has on codemirror-graphql is at the parsing level (syntax highlighting will work now, for example).

Then, in the next PR we can add autocomplete, and simplify the codemirror mode in the process (as per the comment) hopefully!

@acao acao self-assigned this Dec 25, 2020
@acao
Copy link
Member Author

acao commented Dec 28, 2020

merging this without codemirror-graphql support, important to note again. that is coming in a follow up PR

@acao acao merged commit c4cba85 into main Dec 28, 2020
@acao acao deleted the feat/interfaces-interfaces branch December 28, 2020 13:53
@acao acao added this to In progress in 3. Complete LSP Features via automation Jan 26, 2021
@github-actions github-actions bot mentioned this pull request Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants