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

[Language Server] Support more languages #1147

Closed
wants to merge 6 commits into from

Conversation

zth
Copy link
Contributor

@zth zth commented Dec 22, 2019

This PR introduces support for more source languages in the Language Server than js and raw graphql files. It does the following:

  1. Adds support for jsx, ts, tsx and re (ReasonML)
  2. Move all source extraction to use regex's, removing the previous findGraphQLTags that used babel to parse and extract tags from the js documents.
  3. Adds bail early file filters for all languages, in an attempt to ensure some performance.

Adding support for other languages should be fairly trivial as long as extracting sources from that language's files lends itself well to using regex's. For example, the Apollo Language Server (which started out as a fork from this LS), uses the regex-based approach, and supports Python, Dart and probably some more languages as well, in a rather straight forward way by using regex's.

Discussion points

Is using regex's a good idea?

The rationale for using regex's is that extracting GraphQL sources from tags in documents is pretty straight forward. While there's great support for parsing and extracting sources from JavaScript and TypeScript using an AST-based approach, this probably won't be the case for other languages (think Python, ReasonML etc).

Extracting sources feel straight forward and easy enough that a regex based approach can make sense. But this is just my opinion, what do you feel?

Should we default to including all languages or should it be opt in?

Currently with this PR, all supported files/languages are included when traversing the project and extracting GraphQL operations, but one could make the case supporting other things than js and graphql should be opt in. I've added a file filter for each language to ensure we can bail early on files that don't contain any operations, but I'm interested in hearing what you think about including all supported languages by default.

@codecov
Copy link

codecov bot commented Dec 22, 2019

Codecov Report

Merging #1147 into master will increase coverage by 0.42%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
+ Coverage   46.22%   46.64%   +0.42%     
==========================================
  Files          64       65       +1     
  Lines        3003     2995       -8     
  Branches      650      640      -10     
==========================================
+ Hits         1388     1397       +9     
+ Misses       1363     1350      -13     
+ Partials      252      248       -4
Impacted Files Coverage Δ
...hql-language-service-server/src/GraphQLWatchman.js 0% <ø> (ø) ⬆️
...ql-language-service-server/src/MessageProcessor.js 42.85% <100%> (-1.17%) ⬇️
...s/graphql-language-service-server/src/languages.js 82.6% <82.6%> (ø)
...aphql-language-service-server/src/languageUtils.js 93.75% <93.75%> (ø)

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 e90f243...07d827c. Read the comment docs.

@acao
Copy link
Member

acao commented Dec 22, 2019

I'm a bit confused, we already have support for js and jsx? Did you add support via something other than babel?

@zth
Copy link
Contributor Author

zth commented Dec 23, 2019

@acao Hmm, like I said in the PR comment, I added support via regex's instead of babel. Now there's no reason babel couldn't still be used for js/jsx, but supporting ts/tsx would need some other mechanism to work (using the TS parser, or Babel with TS support, which may produce another type of AST that might or might not work with the current findGraphQLTags). Since the regex based approach works the same for js/ts (which the babel approach does not necessarily do), I moved both of them to the regex approach. That can of course be reverted back to using babel again if that's desirable.

Re jsx support: I haven't been able to make that work locally, and I think it's because the current code treats any file that doesn't end in exactly .js like a raw .graphql file:

if (extname(uri) === '.js') {
if (
text.indexOf('graphql`') === -1 &&
text.indexOf('graphql.experimental`') === -1 &&
text.indexOf('gql`') === -1
) {
return [];
}
const templates = findGraphQLTags(text);
return templates.map(({ template, range }) => ({ query: template, range }));
} else {
const query = text;
if (!query && query !== '') {
return [];
}
const lines = query.split('\n');
const range = new Range(
new Position(0, 0),
new Position(lines.length - 1, lines[lines.length - 1].length - 1),
);
return [{ query, range }];
}

@acao
Copy link
Member

acao commented Dec 23, 2019

@zth would you be interested in replacing the babel parser as an experiment in how far you can take this new approach?

@zth
Copy link
Contributor Author

zth commented Dec 23, 2019

@acao I believe that's what I've already done in this PR, or maybe I'm misunderstanding what you mean? This PR removes the use of the Babel parser to extract GraphQL operation from JS files and replaces it with using regex to extract the source instead.

@acao
Copy link
Member

acao commented Dec 23, 2019

@zth I'm just confused because you didnt remove findGraphQLTags but i see now, that this totally replaces it, and doesn't remove the dependencies. its a great POC!

for the full on PR, we will do this in typescript and add a bunch more tests to ease out edgecases, for the many ways template tags can be done in javacsript or typescript for example. maybe add a couple more languages if it's not too difficult. and then in that or a later pass, the ability to provide your own regexes as we were discussing!

so excited.

now remember, we will be moving all of this to the client and removing watchman from the server completely. so all of this work will come in super handy for creating a language service client abstraction!

https://code.visualstudio.com/api/references/vscode-api#workspace

@zth
Copy link
Contributor Author

zth commented Dec 23, 2019

findGraphQLTags is actually removed: https://github.com/graphql/graphiql/pull/1147/files#diff-94b4e244f58f1b38bdc5d1c4e2606d0f

Doing it in TS and adding more tests sounds great! I guess we need #1138 to land before that can happen though, right?

@acao
Copy link
Member

acao commented Dec 23, 2019

@zth yes indeed! #1138 will happen, so i think we can abstract this into a client library for #1146

@acao
Copy link
Member

acao commented Mar 21, 2020

@zth why don't you introduce this to it's own module for now in the LSP utils? I might be doing the same for our own parsing logic, moving it to utils package so that monaco mode can re-use it.

@acao acao closed this 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.

2 participants