Make updates to be spec-compliant to the language server protocol #8

Merged
merged 1 commit into from Feb 1, 2017

Projects

None yet

2 participants

@asiandrummer
Contributor
asiandrummer commented Feb 1, 2017 edited

closes #6.

Inspired by the language server protocol, this PR adds supports to match the mentioned protocol specifications. Notable changes are:

  • New:
    • Adds methods to be invoked from the client using IPC protocol
      • File events (didOpen, didClose, didSave, didChange)
      • Implemented GraphQL diagnostics and autocomplete suggestions (textDocument/publishDiagnostics and textDocument/completion)
    • Added a way to find .graphqlrc configuration file
    • Added support to IPC transport communication (src/server/MessageProcessor.js)
      • Added support to VSCode client
  • Changes/fixes:
    • Changed related flow types and type references to match the protocol spec
    • Updated README to match the current implementations
@asiandrummer asiandrummer requested review from wincent and leebyron Feb 1, 2017
@asiandrummer asiandrummer merged commit 820b03c into master Feb 1, 2017
@wincent

I know this already got merged, but submitting anyway.

-GraphQL Language Service provides an interface for building GraphQL language services for IDEs. Currently supported features include:
-- Diagnostics (GraphQL syntax linting/validations)
-- Autocomplete suggestions
+GraphQL Language Service provides an interface for building GraphQL language service for IDEs.
@wincent
wincent Feb 1, 2017 Contributor

Should still be "for building GraphQL language services for IDEs" (plural services), because grammar. Or if you prefer, "for powering GraphQL language features in IDEs".

-- Autocomplete suggestions
+GraphQL Language Service provides an interface for building GraphQL language service for IDEs.
+
+A subset of supported features of GraphQL language service and GraphQL language server implementation are both specification-compliant to [Microsoft's Language Server Protocol](https://github.com/Microsoft/language-server-protocol), and will be developed to fully support the specification in the future.
@wincent
wincent Feb 1, 2017 Contributor

This sentence is hard to read. "GraphQL language service and GraphQL language server implementation" is confusing. Could we simplify this to:

Partial support for Microsoft's Language Server Protocol is in place, with more to come in the future.

+ -h, --help Show help [boolean]
+ -t, --text Text buffer to perform GraphQL diagnostics on.
+ Will defer to --file option if omitted.
+ This option is always honored over --file option.
@wincent
wincent Feb 1, 2017 Contributor

I'd collapse these two into a single line:

Overrides the --file option, if any.
+ autocomplete suggestions.
+ If omitted, the last column number will be used.
+ [number]
+ -c, --configDir A directory path where .graphqlrc configuration object is
@wincent
wincent Feb 1, 2017 Contributor

This sentence is missing a trailing period, but I think we can make it simpler anyway:

Path to the .graphqlrc configuration file.
+ [number]
+ -c, --configDir A directory path where .graphqlrc configuration object is
+ Walks up the directory tree from the provided config
+ directory, or the current working directory, until
@wincent
wincent Feb 1, 2017 Contributor

"until" → "until a"

-) : Promise<Array<GraphQLAutocompleteSuggestionType>> {
- throw new Error('RPC stub');
-}
+GraphQL Language Server uses [JSON-RPC](http://www.jsonrpc.org/specification) to communicate with the IDE servers to perform language service features. The language server currently supports two communication transports: Stream (stdio) and IPC. For IPC transport, the reference guide to be used for development is [the language server protocol](https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md) documentation.
@wincent
wincent Feb 1, 2017 Contributor

Could delete "to perform language service features" (it's already implied).

Suggest: "the language server protocol" → "Microsoft's language server protocol" to make things clearer.

-): Promise<DefinitionQueryResult> {
- throw new Error('RPC stub');
-}
+For each transports, there is a slight difference between both JSON message format, especially in how the methods to be invoked are defined - below are the currently supported methods for each transports (will be updated as progresses are made):
@wincent
wincent Feb 1, 2017 Contributor

"each transports" → " each transport" (in two places)

"difference between both JSON message format" → "difference in JSON message format"

"progresses are" → "progress is"

+| Diagnostics | `getDiagnostics` | `textDocument/publishDiagnostics` |
+| Autocompletion | `getAutocompleteSuggestions` | `textDocument/completion` |
+| Outline | `getOutline` | Not supported yet |
+| Go-to definition | `getDefinition` | Not supported yet |
@wincent
wincent Feb 1, 2017 Contributor

nit: can we make the trailing bars line up?

@asiandrummer
asiandrummer Feb 1, 2017 Contributor

That one's a little tricky as the lines are too long I think - markdown doesn't care for it though, so I think we're good here?

@wincent
wincent Feb 1, 2017 Contributor

Yeah, I know it will render correctly to HTML, but one of the ideas of Markdown is that the plain-text should look good too. I personally think its ok if the lines here are long (in fact, we're already not hard-wrapping the lines in the file, so we already have lots of long lines).

@@ -52,6 +52,14 @@ const {argv} = yargs
'If omitted, the last column number will be used.\n',
type: 'number',
})
+ .option('c', {
+ alias: 'configDir',
+ describe: 'A directory path where .graphqlrc configuration object is\n' +
@wincent
wincent Feb 1, 2017 Contributor

See comments above on README.

+ * If the file isn't present in the provided directory path, walk up the
+ * directory tree until the file is found or it reaches the root directory.
+ */
+export async function findGraphQLConfigDir(dirPath: Uri): Promise<?string> {
@wincent
wincent Feb 1, 2017 Contributor

Why is this async? I can't see any async calls in it.

+export async function findGraphQLConfigDir(dirPath: Uri): Promise<?string> {
+ let currentPath = path.resolve(dirPath);
+ let filePath;
+ while (currentPath.length > 1) {
@wincent
wincent Feb 1, 2017 Contributor

This looks like it will fail if currentPath tops out at "/" (length 1), so we'd fail to find "/.graphqlrc", wouldn't we? (I know this is likely, but...)

@@ -0,0 +1,419 @@
+/**
+ * Copyright (c) Facebook, Inc.
@wincent
wincent Feb 1, 2017 Contributor

Are we supposed to have a year range in here? (Looking in another file, I see we don't, but in other projects I've seen that we do.)

@asiandrummer
asiandrummer Feb 1, 2017 Contributor

I've double-checked this and I think we intentionally omitted the year range here

@wincent
wincent Feb 1, 2017 Contributor

Ok. For reference, we use years in Relay, graphql-js, and quite a few others, but I am happy to skip it here if our experts say it's ok to do so.

+
+import {findGraphQLConfigDir} from '../config/GraphQLConfig';
+
+import {Position} from '../utils/Range';
@wincent
wincent Feb 1, 2017 Contributor

I don't get the blank lines here. What's the pattern? (group per directory?) Not clear on what the sort order is either.

@asiandrummer
asiandrummer Feb 1, 2017 Contributor

Usually I tried to alphabetically sort imported modules, and group by type imports/named/local with the whitespaces in between. I think I went a bit insane for this diff though - I'll make sure to sort accordingly whenever I see it

+ }
+ }
+ break;
+ case '$/cancelRequest':
@wincent
wincent Feb 1, 2017 Contributor

(Too lazy to read the protocol) What's the significance of the $ here?

@asiandrummer
asiandrummer Feb 1, 2017 Contributor

Not sure, I don't think it says anything about it

@wincent
wincent Feb 1, 2017 edited Contributor

Yeah, I looked too and see nothing. It is a bit odd as it is the only message that uses $ in the name. ¯\(ツ)

- const graphQLCache = await getGraphQLCache(configDir || process.cwd());
- const languageService = new GraphQLLanguageService(graphQLCache);
+ if (message.id !== undefined || message.id !== null) {
@wincent
wincent Feb 1, 2017 Contributor

Could be just:

if (message.id != null) {
@asiandrummer
Contributor

@wincent thanks lots for reviewing! I will open another pull request accommodating your review comments

@wincent wincent deleted the language-server-protocol branch Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment