fix: Fix vscode-graphql-syntax’s grammar to support string literals on separate lines

[kitten]

Summary

We're currently working on gql.tada, which is mostly compatible with the expected syntax of graphql calls.
However, while getting this in users hands we noticed that a specific grammar that Prettier formats graphql() calls into fails to be matched by the Textmate grammar in vscode-graphql-syntax.

Specifically, when a GraphQL string literal is placed on a new line, the begin pattern of the rules can't match. This is a limitation in Textmate grammars, as it turns out, and we can't even manually match \n patterns.

I'm not an expert at Textmate grammars, but, in theory, a grammar rule that matches GraphQL calls as a lookbehind, then matches the inner string literal separately, should do the trick.

Before	After

Follow-up questions

• Is there a test suite / sample files for this we can check for regressions?
• Does anyone here know more about Textmate grammars to proof read this change?

Set of changes

• Try out newline matches in begin rule (DELETED CHANGE)
• Add separate rule for matching graphql(...) calls then matching string literals inside
• Combining redundant rules into one (move generic over)
• Update snapshots (remove TODO: fix this) and add more grammar checks for JS

[changeset-bot]

🦋 Changeset detected

Latest commit: 84777a2

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

This PR includes changesets to release 1 package

Name	Type
vscode-graphql-syntax	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]

The committers listed above are authorized under a signed CLA.

• ✅ login: kitten / name: Phil Pluckthun (3d615ca, 62f8746, 90be966, 4edd5c0, af534ac, ba22e03, ce75cdd, bac851b, 50d8e3c, 84777a2, eae5fea, f8469c2, 057f22e, 133b2af)

[acao]

@kitten thanks for this! to answer your questions, see the syntax extension readme

[acao]

also, can you add to the test fixtures the test case you originally hoped to cover? I don't see it present in your PR

[kitten]

@acao: Cheers! I forgot to add some more cases for the original failing matches ✌️ Those should be added in: 057f22e
Regarding the readme, I did test the extension manually as well, so the screenshots should cover that 🤔

[acao]

@kitten amazing work here! looking forward to getting this released

I had to release a quick hotfix for the triple double quote syntax """ comment """, I hope you don't mind merging that change. for now we aren't as concerned with graphql(" query { id }") type scenarios because they are much less common than the """ comment """ syntax. if you can make both of those worlds happy, then heck yeah that's awesome! but if not, it's no bother

also, one last nice to have - maybe confirm a few weird pre-auto-formatted variations just to be sure we don't deprecate it somehow in the future. it's nice to have the strings always working no matter whether auto-formatting (prettier, etc) is used or has been applied

for example, corner cases like this

const query = graphql(
` query {
    something
    }
 `);

our LSP server uses normal AST parsing ofc, but with the textmate regexes so many things can go wrong haha

if you don't have time to add corner cases, I can add them after of course!

[kitten]

@acao Merged that back 👍 I'm not too concerned either. No idea what the syntax currently says for single line graphql(" \"\"\"comment\"\"\" query { id }") cases, but the original case that this PR addresses is Prettier putting a template literal (or 'query' string) onto their own lines, which broke the previous syntax definition.

Given that double-quotes " inside a GraphQl string would have to be quoted, Prettier likely would reformat the string with single quotes. And that even assumes they're single line. So, either way, we should be fine there.

re. corner cases, I'm not too concerned about adding all kinds of newline cases to the tests. There's one, which basically represents all possible cases; i.e. \n[ ]{2} before the start of a string. That's equivalent to “all” cases, so to speak, since Textmate grammar can't match past newlines, so the call syntax (e.g. graphql(\n ... \n)) is what we ultimately have to check against.
So, all good from my end 👍

Edit: Just an example of why I think this is covered 🤔 Here's an alteration to the test.js file that includes a ("...") case formatted by Prettier:

-const graphql = graphql(
-  "\"\"\"comment\"\"\"\nquery($id: ID!) { test }",
-  [var1, var2]
-);
+const graphql = graphql('"""comment"""\nquery($id: ID!) { test }', [
+  var1,
+  var2,
+]);

[kitten]

Just came back from some holidays, so coming back to this, let me know if there's anything else we want to do here, or if we're good to go ✌️❤️

[acao]

since Textmate grammar can't match past newlines
I'm not sure if this is true, if I understand?

otherwise though it looks great! i will take a closer look and see about merging it soon

[acao]

actually, can you add some more test cases to test-js.md as well? it should work transparently

[kitten]

@acao: re. not matching past multiple lines, I found a more detailed discussion here, which is where I initially ended up for this PR. Basically, what I meant was that begin and end can't literally include matches for a newline character, since the matches are separated line by line and that's why the new rules match the function call first.

I applied some of the additions to test-js.md. I don't think the fixtures there test for code matches, but it's now covering the relevant cases for future changes to the syntax 👍

[xamir82]

Just stumbled into this issue. Would love to see this merged soon.

[acao]

indeed, the fixtures in markdown do look for matches to the tokenized js/graphql results down to each charactrer! it helps us make sure that these cases continue to work when markdown characters might conflict with our js/ts template tags. as you can see, the markdown grammar is using the grammar you are modifying heavily!

in the future, if you add the test cases to the end of the fixture file, the diff makes more sense, but that is often the case with snapshot serializers, including our custom textmate token serializer

I hope to merge this and some other PRs today on my day off, so be on the lookout!

[acao]

awesome work, this is really comprehensive!

the regression snapshots will help tremendously if someone tries a "bugfix" for their corner case that breaks this scenario, which is exactly why we set up this snapshot suite!

this is a fantastic fix contribution to our ecosystem and many others that use our extension, so thank you @kitten !

[acao]

There appears to be something awry with github actions, where it's not reporting the run status (for me at least), and the log output formatting is broken, but manually checking each job, it looks fine! merging now, thanks again!

[acao]

bad news - it appears this (missing) github actions event bug is also not triggering the changeset bot?

good news - once the issue recovers, we will still be able to release the changes without a problem with the next merge to main

[aradalvand]

@acao Sorry for being annoying but when is this (i.e. v1.3.3) going to be released?
Thank you.

[kitten]

It has already been released: https://marketplace.visualstudio.com/items?itemName=GraphQL.vscode-graphql-syntax

[aradalvand]

This seems to have caused a regression, showcased in #3542.

[acao]

no worries @kitten this happens! lets double check the snapshot suite and make sure we have the right fixtures for this case and/or see if the snapshot change reveals this bug. we can re-release no problem! I really need to create a pre-release flow for these extensions, but that's another discussion

[acao]

@aradalvand 0.9.2 is now released, and removes this regression by reverting the 0.9.1 release. thanks for reporting!

[acao]

@kitten I think the issue we can see from the snapshot side is that the end ` and ; i think are being tokenized by graphql somehow? So i think the query remains unterminated and attempts to highlight the rest of the file from the first graphql tag to the end of the file. i would look closely at the serializer results, and follow the readme instructions for performing manual builds to visually confirm the outcomes, where you open the fixture files and can confirm that everything is as expected

[kitten]

I'll take a look! I've tested this before in VSCode, but I'm guessing there's either a case that isn't covered or I made a mistake while resolving conflicts 🤔

[acao]

yeah, that's what I guessed as well! also, my bad for not manually confirming everything immediately pre-release myself, that's my job 😆

[kitten]

Not your job and not your job solely if you do consider it your job ❤️ I did confirm and double-check this, but it was a subtle mistake where the pattern wasn't quite matching, because the positive lookbehind failed, and was neither applying ts/js/etc highlighting inside the function call expression, nor was it then matching correctly after the pattern, since the pattern causes an error.

At least that's my assumption.
It's easy to miss because the tests don't contain the VSCode tmLanguage patterns for JS/TS themselves, which makes it hard to see that this is happening, because it does at first glance all look correct.

This does make me think that while this may sound silly, visual snapshots would be really helpful in the future, if the VSCode team is willing to provide such a tool 🤔

I found the issue and will open a follow-up PR

[kitten]

Fix is in a new PR here ❤️ #3545
Sorry for messing this up!