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: export more functions #1264

Merged
merged 3 commits into from Apr 4, 2020
Merged

Conversation

yoshiakis
Copy link
Collaborator

@yoshiakis yoshiakis commented Jan 24, 2020

Notice: I'll PR this change once #1265 has completed since I want to rebase the branch at this PR on master branch after #1265 has finished.

This change exports more functions from graphql-language-service-interface so codemirror-graphql can use them, too (#902 ).

@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #1264 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1264   +/-   ##
=======================================
  Coverage   58.98%   58.98%           
=======================================
  Files          60       60           
  Lines        3023     3023           
  Branches      806      806           
=======================================
  Hits         1783     1783           
  Misses       1194     1194           
  Partials       46       46           
Impacted Files Coverage Δ
...ervice-interface/src/getAutocompleteSuggestions.ts 70.45% <ø> (ø)
...anguage-service-interface/src/autocompleteUtils.ts 91.52% <100.00%> (ø)

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 c57a374...91cb5e0. Read the comment docs.

@acao
Copy link
Member

acao commented Jan 24, 2020

hey thanks! sure this works, though ideally soon we should be instantiating the GraphQLLanguageService in codemirror instead as we will be doing with monaco, so we can leverage the class instance for a bunch of useful things

@acao
Copy link
Member

acao commented Jan 24, 2020

also, with lerna's conventional commits implementation, you don't need to indicate the package for scope, as this will automatically generate a change to only the interface changelog anyways. so just feat is fine

@acao
Copy link
Member

acao commented Jan 24, 2020

I take that back, I see now that codemirror uses the ContextToken argument which the LanguageService does not expose.

this should be fine then!

again though, we should combine all of the types, no need for codemirror specific types except for ContextToken which vscode doesnt even have a concept of context tokens? so the word Codemirror shouldn't need to appear in the names of any types or interfaces!

@@ -247,6 +247,16 @@ export type ContextToken = {
style: string;
};

export type ContextTokenForCodeMirror = {
Copy link
Member

Choose a reason for hiding this comment

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

ContextToken is only used by Codemirror, so lets combine this into one type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't ContextToken used inside graphql-language-service-interface even if you do with vscode?

export type CompletionItem = CompletionItemType & {
isDeprecated?: boolean;
deprecationReason?: string;
};

export type CompletionItemForCodeMirror = {
Copy link
Member

@acao acao Jan 28, 2020

Choose a reason for hiding this comment

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

what is diffferent about this type? do you just need to make it deprecationReason?: string | null?

Copy link
Collaborator Author

@yoshiakis yoshiakis Jan 29, 2020

Choose a reason for hiding this comment

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

In a successive PR, getAutocompleteSuggestions for CodeMirror returns an object that only contains properties label, type, documentation, isDeprecated, deprecationReason. It doesn't contain many of properties of CompletionItemType and I don't have a plan to add those properties, so I thought using a type designated for it, which is CompletionItemForCodeMirror, it is easier to understand what getAutocompleteSuggestions for CodeMirror actually returns.

@acao
Copy link
Member

acao commented Apr 2, 2020

@yoshiakis coming back to this and realizing that a) our flow types are again woefully out of date and b) the monaco PR is about to lead to some LSP interface changes.

@acao acao closed this Apr 4, 2020
@yoshiakis
Copy link
Collaborator Author

@acao Thank you for the information! This PR was one of PRs to be needed to fix #868. However, GraphiQL and GraphQL Playground seem to use monaco instead of codemirror-graphql in the near future so I'm thinking to give up to fix it.

@acao
Copy link
Member

acao commented Apr 4, 2020

@yoshiakis oh my bad, i think i was mistaken. let me double check this PR

@acao acao reopened this Apr 4, 2020
@acao acao marked this pull request as ready for review April 4, 2020 16:49
@acao
Copy link
Member

acao commented Apr 4, 2020

looks good to me!

@acao acao changed the title [WIP]feat(gql-interface): export more functions feat(gql-interface): export more functions Apr 4, 2020
@acao acao changed the title feat(gql-interface): export more functions feat: export more functions Apr 4, 2020
@acao acao merged commit bd01fdd into graphql:master Apr 4, 2020
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 this pull request may close these issues.

None yet

2 participants