stardog-union / stardog-language-servers

Language Servers for Stardog Languages
Other
30 stars 6 forks source link

Basic GraphQL language server with diagnostics, hover, autocomplete #16

Closed jmrog closed 5 years ago

jmrog commented 5 years ago

Close #15.

Implementation is intentionally nearly identical to the ShaclLanguageServer's implementation -- identical patterns used there and here.

jmrog commented 5 years ago

I'm having trouble using Studio with this. Is it because of the conflicting package name? Even when I change that to stardog-graphql-language-server in this package's package.json and the dep I add to package.json in your Studio branch it fails (after building, linking, etc) at yarn in Studio. Does it work for you?

Yeah, it works for me. You need to do a bunch of linking to make it work. First, you have to run yarn inside of the stardog-language-serversrepo. Next, you have to go into the stardog-language-utils directory and run yarn build followed by yarn link. Then you have to go into the graphql-language-server directory and run yarn link stardog-language-utils followed by yarn build and yarn link. At that point, you will have to run yarn link stardog-language-utils and yarn link graphql-language-server inside of the Studio repo, and it should work.

brianru commented 5 years ago

Thanks, @jmrog ! Giving that a shot.

brianru commented 5 years ago

Now getting this Error: Unresolved node modules: chevrotain, escape-string-regexp, lodash.uniqby, millan

brianru commented 5 years ago

blowing away node_modules helped 🤕

brianru commented 5 years ago

@jmrog I got an infinite loop error!

image
brianru commented 5 years ago

Documenting stuff -- I'm only getting syntax completion when immediately preceding a closing }: must_be_immediately_preceding_closing_brace

or when immediately preceding or succeeding an opening { or preceding a type: must_be_adjacent_to_opening

I guess what's missing is in the spaces following a field but not immediately preceding {

and why would I ever want to replace a field with ...? yet that's a suggestion

jmrog commented 5 years ago

@jmrog I got an infinite loop error!

image

Any idea how you got this to happen? Was it before you sorted out all the linking, etc.? I can't get this to happen so far no matter what I do.

brianru commented 5 years ago

@jmrog yep, reproduced! infinite_loop_error

jmrog commented 5 years ago

@jmrog yep, reproduced! infinite_loop_error

Yeah, thanks, I just managed to trigger it, too. Seems to happen only on inline fragment. This sucks because that rule directly encodes the GraphQL spec. Will see if I can figure out a workaround.

brianru commented 5 years ago

Why aren't directives being suggested when those suggestions would be most useful? [@jmrog feel free to not answer right away...I want to tie this back to the impl] directives

directives_more

jmrog commented 5 years ago

@jmrog yep, reproduced! infinite_loop_error

Yeah, thanks, I just managed to trigger it, too. Seems to happen only on inline fragment. This sucks because that rule directly encodes the GraphQL spec. Will see if I can figure out a workaround.

Apparently others are facing this same(?) issue: https://github.com/prisma/vscode-graphql/issues/76

brianru commented 5 years ago

@jmrog yep, reproduced!

Yeah, thanks, I just managed to trigger it, too. Seems to happen only on inline fragment. This sucks because that rule directly encodes the GraphQL spec. Will see if I can figure out a workaround.

Apparently others are facing this same(?) issue: prisma/vscode-graphql#76

that sucks. I wonder what it's actually affecting

jmrog commented 5 years ago

@jmrog yep, reproduced!

Yeah, thanks, I just managed to trigger it, too. Seems to happen only on inline fragment. This sucks because that rule directly encodes the GraphQL spec. Will see if I can figure out a workaround.

Apparently others are facing this same(?) issue: prisma/vscode-graphql#76

that sucks. I wonder what it's actually affecting

Yeah, not sure. It doesn't really seem to have any practical implications from what I can tell (yet).

jmrog commented 5 years ago

@jmrog yep, reproduced!

Yeah, thanks, I just managed to trigger it, too. Seems to happen only on inline fragment. This sucks because that rule directly encodes the GraphQL spec. Will see if I can figure out a workaround.

Apparently others are facing this same(?) issue: prisma/vscode-graphql#76

that sucks. I wonder what it's actually affecting

Yeah, not sure. It doesn't really seem to have any practical implications from what I can tell (yet).

Confirmed that it is exactly the same issue as in prisma/vscode-graphql#76. The GraphQL grammar does not "advance" before re-entering the SelectionSet rule in the rare case where a Spread token is followed immediately by another SelectionSet. In other words, this is valid GraphQL:

{
  ...{
    ...{
      ...{
        ...{ # keep going into infinity so that the SelectionSets never actually close

This is built into the GraphQL grammar, abbreviated here:

SelectionSet ::= '{' Selection+ '}'
Selection ::= ( Field | InlineFragment | FragmentSpread )
InlineFragment ::= '...' TypeCondition? Directives? SelectionSet

So technically you can infinitely go: SelectionSet --> InlineFragment --> '...' SelectionSet --> InlineFragment --> '...' SelectionSet --> ...

I'm not sure whether this is intended but I could potentially raise an issue on the repo for the GraphQL spec.

brianru commented 5 years ago

The error is only thrown in chevrotain < 4.4.0 https://github.com/SAP/chevrotain/issues/958

So I suppose it's a no-op for us.

brianru commented 5 years ago

@jmrog So, if I understand this correctly, I have no issues with this code itself. I don't find the completions to be all that helpful as you often have to start writing a syntax form to get useful completions, it often doesn't suggest what you can do, but is it right that that is determined by the parser in millan?

ex: this doesn't suggest adding a directive or another field or treating that field as a predicate so suggesting { ... } to add sub-fields

image
jmrog commented 5 years ago

@jmrog So, if I understand this correctly, I have no issues with this code itself. I don't find the completions to be all that helpful as you often have to start writing a syntax form to get useful completions, it often doesn't suggest what you can do, but is it right that that is determined by the parser in millan?

ex: this doesn't suggest adding a directive or another field or treating that field as a predicate so suggesting { ... } to add sub-fields

Yes, that's right. The current autocomplete is intended as the MVP, to be made smarter later. It currently just comes straight from chevrotain via millan, using the built-in computeContentAssist method that runs on the grammar.

Even so, I agree with you about the usefulness of this autocomplete and think it could be made at least somewhat better if I modify the GraphQL grammar slightly, so maybe I'll do that in millan before releasing. The core problem is that the GraphQL spec doesn't specify directives as special tokens, but instead has a single RegEx that covers all directives, so chevrotain/millan can't provide autocomplete for any particular directive. If I refactor the grammar to have explicit tokens for (some) directives, it'll at least be a better experience there.

brianru commented 5 years ago

Got it. Thanks! This is slowly starting to make sense to me :) I'll approve this, but feel free to assign me to a millan PR first, or no problem if you just want to get this out -- it's better than nothing.