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

convert LSP Server to Typescript, remove watchman #1138

Merged
merged 6 commits into from
Jan 18, 2020
Merged

Conversation

acao
Copy link
Member

@acao acao commented Dec 19, 2019

0 compile errors! now just tell me what i did wrong 😂

  • - convert graphql-language-service-server to typescript
  • - convert graphql-language-service to typescript
  • - stumbled upon, fixed bug in graphql-language-service-interface with missing deprecated fields in autocomplete? https://github.com/graphql/graphiql/pull/1138/files#diff-564f41e59f16fb85571ebd58e501253eR106
  • - two @ts-ignore lines in server still
  • - still some tsc compile issues in CLI client lib?
  • - we need more tests for LSP server badly, still coming in between 30-40% it looks like

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #1138 into master will increase coverage by 0.1%.
The diff coverage is 58.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1138     +/-   ##
=========================================
+ Coverage   53.74%   53.85%   +0.1%     
=========================================
  Files          76       75      -1     
  Lines        3414     3435     +21     
  Branches      650      662     +12     
=========================================
+ Hits         1835     1850     +15     
- Misses       1361     1448     +87     
+ Partials      218      137     -81
Impacted Files Coverage Δ
...ackages/graphql-language-service-utils/src/file.ts 93.18% <ø> (ø) ⬆️
...l-language-service-interface/src/getDiagnostics.ts 91.07% <ø> (ø) ⬆️
...kages/graphql-language-service-server/src/index.ts 0% <ø> (ø)
packages/graphql-language-service/src/client.ts 0% <0%> (ø)
...graphql-language-service-server/src/startServer.ts 0% <0%> (ø)
...ge-service-interface/src/GraphQLLanguageService.ts 43.95% <0%> (ø) ⬆️
packages/graphql-language-service/src/cli.ts 0% <0%> (ø)
...anguage-service-interface/src/autocompleteUtils.ts 92.53% <100%> (ø) ⬆️
...ervice-interface/src/getAutocompleteSuggestions.ts 69.39% <100%> (ø) ⬆️
...raphql-language-service-server/src/stringToHash.ts 11.11% <100%> (ø)
... and 13 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 adb73f5...493b941. Read the comment docs.

@acao acao force-pushed the chore/convert-ls-server branch 3 times, most recently from 480c1c7 to 7e771ed Compare December 21, 2019 04:50
@acao acao requested review from a team and lostplan December 23, 2019 13:39
@acao
Copy link
Member Author

acao commented Dec 23, 2019

@divyenduz would you like to give this a review when you get a chance in January?

@acao acao changed the title [WIP] convert LSP Server to Typescript convert LSP Server to Typescript Dec 23, 2019
@acao
Copy link
Member Author

acao commented Dec 23, 2019

oops forgot to remove the WIP prefix last week

@divyenduz
Copy link
Contributor

@acao Sure, I will do that, is the public interface completely same? Apart from review, I can also integrate this with the GraphQL extension maybe to test.

@acao
Copy link
Member Author

acao commented Dec 23, 2019

@divyenduz indeed! that would be great. that's the goal, the interface should be completely the same, though slightly adopted to use more built in vscode types.

@acao
Copy link
Member Author

acao commented Dec 27, 2019

I plan on keeping the watcher in for this go around, though it's been a lot of trouble. Is the GraphQL extension using the watcher or a vscode extension client watcher?

@divyenduz
Copy link
Contributor

Is the GraphQL extension using the watcher or a vscode extension client watcher?

It uses the watcher from this project but that has been troublesome, feel free to remove it, the extension can be mended later :)

@acao
Copy link
Member Author

acao commented Dec 28, 2019

I think I will keep it for this PR, to keep it as close to a refactor as possible. going to start creating PRs off of this one

@acao
Copy link
Member Author

acao commented Jan 2, 2020

@divyenduz what do you think? should we just remove it in this PR? it seems that removing it will add its own complexity, so possibly in a successive PR? that way I can make sure the tests here match up

also i noticed that i had actually broken isDeprecated in terms of the filtering logic. I changed it back to extend CompletionItem for isDeprecated in addition to the vscode expected deprecated flag. it will map to the official deprecated flag but still filters values.

The possiblity is that via a configuration option, the deprecated fields could still appear in completion but with the official deprecation notice that will allow vscode to show the deprecation notices if needed. This would be relatively easy to implement as it would only require removing some filter() statements sprinkled throughout interface, as these manual filters are implemented upstream from codemirror or server implementations.

@divyenduz
Copy link
Contributor

@acao Keeping it in this PR, and getting rid of it in a successive PR sounds like a good plan.

@acao acao force-pushed the chore/convert-ls-server branch 2 times, most recently from 9469d49 to 8151dee Compare January 12, 2020 23:00
@acao
Copy link
Member Author

acao commented Jan 18, 2020

@divyenduz exciting update

  • it works great in vscode-graphql (your debug runner setup is very helpful!)
  • I removed the watchman!

@acao acao changed the title convert LSP Server to Typescript convert LSP Server to Typescript, remove watchman Jan 18, 2020
@acao acao merged commit 8e33dbb into master Jan 18, 2020
@divyenduz
Copy link
Contributor

@acao Exciting 🎉

I would like to use this and cut a release in the VSCode extension (after testing it a lot manually), as watchman was a big pain point there. But how do I grab a hold of the alpha? Is there an automatic alpha release?

I know that releasing master is a manual process right now but was wondering if I can get a bite of this code in alpha or something?

@acao
Copy link
Member Author

acao commented Jan 18, 2020

I'm going to go ahead and cut an alpha release of everything. thats a good idea, of cutting an alpha automatically

const namedInputType = getNamedType(typeInfo.inputType as GraphQLType);
if (namedInputType instanceof GraphQLEnumType) {
const values = namedInputType.getValues();
const values: GraphQLEnumValues[] = namedInputType.getValues();
Copy link
Collaborator

@yoshiakis yoshiakis Jan 19, 2020

Choose a reason for hiding this comment

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

GraphQLEnumValues[] (not GraphQLEnumValue[]) is not defined in this file, and without defining a type here, values is implied as GraphQLEnumValue[] from namedInputType.getValues(). I'm wondering if it's just a mistake or is there any reason to define a type for values here?

@acao
Copy link
Member Author

acao commented Jan 19, 2020

good catch! string | undefined will have to do for now

@acao acao deleted the chore/convert-ls-server branch February 23, 2020 04:58
@acao acao added this to the GraphiQL-1.0.0-beta milestone Mar 14, 2020
@acao acao removed the request for review from lostplan May 19, 2020 20:20
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

3 participants