feat: add gql call expressions support #2509

Chnapy · 2022-06-16T18:14:57Z

This PR enables highlight & intellisense on common gql() calls without need to add a #graphql tag or equivalent.

Before:

After:

changeset-bot · 2022-06-16T18:15:00Z

🦋 Changeset detected

Latest commit: c22885b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
vscode-graphql	Patch
graphql-language-service-server	Patch
graphql-language-service-cli	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

linux-foundation-easycla · 2022-06-16T18:15:03Z

The committers listed above are authorized under a signed CLA.

✅ login: Chnapy / name: Richard Haddad (ba26b43, 76a5357)

acao · 2022-06-26T10:40:02Z

@Chnapy this adds highlighting support only fyi! It would seem language support already exists in the LSP server. Can you add a changeset?

codecov · 2022-06-26T10:41:50Z

Codecov Report

Merging #2509 (c22885b) into main (2d91916) will increase coverage by 3.12%.
The diff coverage is 23.64%.

@@            Coverage Diff             @@
##             main    #2509      +/-   ##
==========================================
+ Coverage   65.70%   68.83%   +3.12%     
==========================================
  Files          85       71      -14     
  Lines        5106     4155     -951     
  Branches     1631     1375     -256     
==========================================
- Hits         3355     2860     -495     
+ Misses       1747     1290     -457     
- Partials        4        5       +1

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/lint.ts	`100.00% <ø> (ø)`
packages/codemirror-graphql/src/results/mode.ts	`47.05% <ø> (ø)`
packages/codemirror-graphql/src/utils/hintList.ts	`95.65% <ø> (ø)`
...ckages/codemirror-graphql/src/utils/mode-indent.ts	`0.00% <0.00%> (ø)`
packages/codemirror-graphql/src/variables/mode.ts	`79.48% <ø> (ø)`
packages/graphiql-react/src/editor/whitespace.ts	`100.00% <ø> (ø)`
packages/graphiql-react/src/utility/debounce.ts	`0.00% <0.00%> (ø)`
packages/graphiql-react/src/editor/tabs.ts	`5.76% <5.76%> (ø)`
packages/codemirror-graphql/src/variables/lint.ts	`46.98% <66.66%> (ø)`
packages/codemirror-graphql/src/hint.ts	`94.73% <100.00%> (ø)`
... and 97 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 74aac2c...c22885b. Read the comment docs.

acao · 2022-06-26T10:43:53Z

@Chnapy I haven't seen this pattern used. Usually with graphql-tag and relay and such, it's

const myQuery = gql`
  query {
    field
  }
`

as you can see, even github's ace editor supports this pattern!

whereas, this pattern is not supported in most of the third party ecosystem as you can see:

const myQuery = gql(`
  query {
    field
  }
`)

``` (a

do you have examples of libraries that use this pattern? I'm worried about creating confusion, because if someone used `graphql-tag` or similar libraries this way, it would look like it's working when it would break at runtime (at least for js users)

acao · 2022-06-26T18:21:28Z

@Chnapy my apologies, we can add support for this syntax no problem, I don't need examples, there is no sense holding back innovation.

To clarify, the vscode-graphql grammars only add syntax highlighting support, and only to vscode-graphql client and not the LSP server itself. On the language support side, I'm actually not sure if our implementation of @babel/core findGraphQLTags in graphql-language-service-server, I think it may actually already cover that, and appeared to not work because the grammar did not support syntax highlighting for this yet.

Edit - tests!

There are some tests in packages/graphql-language-service-server/src/__tests/MessageProcessor-test.tsthat act as integration tests, but come to think of it parseDocument and findGraphQLTags have no unit tests! not that you need to add them for this, you can just add a quick one to the aforementioned file if it isn't there already

Let me know how I can help if it needs completion!

Chnapy · 2022-06-26T18:48:02Z

@acao thanks for your feedbacks !

I worked on this add because of a small typescript plugin I'm working on (ts-gql-plugin).
Since I change typing returned by gql calls (TypedDocumentNode instead of DocumentNode) I cannot use template literal calls because of a TS type-safety issue. So I should use gql(`...`) instead of gql`...` .

There is no difference between these 2 ways of doing with graphql-tag. To check that I made a basic test on https://npm.runkit.com/graphql-tag :

require("graphql/package.json"); // graphql is a peer dependency. 
const { gql } = require("graphql-tag")

const a = gql`
    query Foo {
      bar {
        id
      }
    }
`;
  
const b = gql`
    query Foo {
      bar {
        ${'id'}
      }
    }
`;

const c = gql(`
    query Foo {
      bar {
        id
      }
    }
`);
  
const d = gql(`
    query Foo {
      bar {
        ${'id'}
      }
    }
`);

const strA = JSON.stringify(a);
const strB = JSON.stringify(b);
const strC = JSON.stringify(c);
const strD = JSON.stringify(d);

console.log(
a, b, c, d,
strA.length, strB.length, strC.length, strD.length,   // same length
strA === strB, strC === strD, strA === strC  // true
);

--

I admit this is a very specific use-case, based on one random librairy. I thought this add would cost quite nothing in maintainability (?). Also since it is simply possible to call gql() I find nice to handle this case, for projects with specific linter rules for example.

To clarify, the vscode-graphql grammars only add syntax highlighting support, and only to vscode-graphql client and not the LSP server itself. On the language support side, I'm actually not sure if our implementation of @babel/core findGraphQLTags in graphql-language-service-server, I think it may actually already cover that, and appeared to not work because the grammar did not support syntax highlighting yet.

Hmm I see, I think I found the part to work on. I'll pass some time and update this PR if I succeed something 👍

acao · 2022-06-26T19:28:26Z

@Chnapy yes my bad, graphql-tag does work both ways now! a long time ago I think it didn't.

Are we sure the language support works for this syntax?

the visitor you linked to in the last sentence is used for comment tags, which is a different visitor than what we need.

what we need to do is repeat this, I think:

graphiql/packages/graphql-language-service-server/src/findGraphQLTags.ts

Line 173 in a390b40

TaggedTemplateExpression: (node: TaggedTemplateExpression) => {

also see an update to my above comment about where to add (partial) integration test coverage!

Chnapy · 2022-06-30T19:44:47Z

I made few changes on findGraphQLTags, language support works well now following my local testing 👍

I put the logic into CallExpression function to ensure tag name check, and reuse TemplateLiteral logic.

Some tests added in MessageProcessor-test.ts, and I created findGraphQLTags-test.ts as you suggested. In this file I put just few tests, but major part of MessageProcessor tests can be copied to it.

acao · 2022-07-01T02:48:49Z

@Chnapy brilliant work, thanks for this! Looks great so far and I will give it a spin later today

acao · 2022-07-01T09:22:54Z

packages/graphql-language-service-server/src/findGraphQLTags.ts

@@ -100,10 +100,48 @@ export function findGraphQLTags(
  }
  const ast = parsedAST!;

+  const parseTemplateLiteral = (node: TemplateLiteral) => {


this is exactly the abstraction I was hoping for!

acao · 2022-07-01T09:23:07Z

packages/graphql-language-service-server/src/findGraphQLTags.ts

+
+        if (
+          callee.type === 'Identifier' &&
+          getGraphQLTagName(callee) &&


acao

@Chnapy reviewed it locally and everything works great. thanks for all your work on this!

acao · 2022-07-03T12:09:27Z

@Chnapy I think I've found a substantial section of code that is never used within CallExpression visitor. Tests confirm that everything appears to be ok - I just wonder what test case I'm missing that this was supposed to cover?

acao · 2022-07-03T13:26:28Z

@stonexer if you'd like to take a look, and make sure you can't find a test case that I'm overlooking by deleting this seemingly unused functionality??

acao · 2022-07-03T14:37:11Z

merged! thanks @Chnapy ! 🥳

saihaj approved these changes Jun 19, 2022

View reviewed changes

Chnapy changed the title ~~feat: add with-parenthese gql calls support~~ feat: add gql call expressions support Jun 30, 2022

acao reviewed Jul 1, 2022

View reviewed changes

packages/graphql-language-service-server/src/findGraphQLTags.ts

if (

callee.type === 'Identifier' &&

getGraphQLTagName(callee) &&

Copy link

Member

acao Jul 1, 2022

Choose a reason for hiding this comment

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

💯

acao approved these changes Jul 3, 2022

View reviewed changes

Chnapy and others added 4 commits July 3, 2022 10:59

chore: fix watch-vscode broken script

65513b4

feat: add with-parenthese gql calls support

eab7bed

feat: add gql call expressions language support

c5acfaf

update changelog

aeb84dc

acao force-pushed the highlight-gql-with-parenthese branch from a3ce572 to aeb84dc Compare July 3, 2022 09:01

remove unused embedded graphql lookup

c22885b

acao merged commit 737d418 into graphql:main Jul 3, 2022

This was referenced Jul 3, 2022

Version Packages #2529

Merged

add more tests for findGraphQLTags, fix cache/schema tests #2533

Merged

Chnapy mentioned this pull request Jul 15, 2022

Remove #graphql comment tag Chnapy/ts-gql-plugin#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add gql call expressions support #2509

feat: add gql call expressions support #2509

Chnapy commented Jun 16, 2022

changeset-bot bot commented Jun 16, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jun 16, 2022 •

edited

Loading

acao commented Jun 26, 2022

codecov bot commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading

Chnapy commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading

Chnapy commented Jun 30, 2022

acao commented Jul 1, 2022

acao Jul 1, 2022

acao Jul 1, 2022

acao left a comment

acao commented Jul 3, 2022

acao commented Jul 3, 2022

acao commented Jul 3, 2022

feat: add gql call expressions support #2509

feat: add gql call expressions support #2509

Conversation

Chnapy commented Jun 16, 2022

changeset-bot bot commented Jun 16, 2022 • edited Loading

🦋 Changeset detected

linux-foundation-easycla bot commented Jun 16, 2022 • edited Loading

acao commented Jun 26, 2022

codecov bot commented Jun 26, 2022 • edited Loading

Codecov Report

acao commented Jun 26, 2022 • edited Loading

acao commented Jun 26, 2022 • edited Loading

Chnapy commented Jun 26, 2022 • edited Loading

acao commented Jun 26, 2022 • edited Loading

Chnapy commented Jun 30, 2022

acao commented Jul 1, 2022

acao Jul 1, 2022

Choose a reason for hiding this comment

acao Jul 1, 2022

Choose a reason for hiding this comment

acao left a comment

Choose a reason for hiding this comment

acao commented Jul 3, 2022

acao commented Jul 3, 2022

acao commented Jul 3, 2022

changeset-bot bot commented Jun 16, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jun 16, 2022 •

edited

Loading

codecov bot commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading

Chnapy commented Jun 26, 2022 •

edited

Loading

acao commented Jun 26, 2022 •

edited

Loading