[RFC] fix: fix block string parsing in language parser

[dwwoelfel]

This is an attempt to fix block string support in GraphiQL.

The goal is to make a query like the following parse properly in GraphiQL:

{
  hasArgs(
    string: """      
      hello
      world
    """
  )
}

It seems like there shouldn't be logic about how block strings are constructed in onlineParser.ts, but I'm having a hard time keeping that knowledge solely in Rules.ts. I'm finding it very tricky to write any logic that requires information from the previous line.

[codecov]

Codecov Report

Merging #1777 (79382a5) into main (cb4debd) will increase coverage by 0.68%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #1777      +/-   ##
==========================================
+ Coverage   65.72%   66.40%   +0.68%     
==========================================
  Files          85       86       +1     
  Lines        5091     5153      +62     
  Branches     1607     1643      +36     
==========================================
+ Hits         3346     3422      +76     
- Misses       1718     1727       +9     
+ Partials       27        4      -23     

Impacted Files	Coverage Δ
...raphql-language-service-parser/src/onlineParser.ts	87.23% <16.66%> (-0.92%)	⬇️
...kages/graphql-language-service-parser/src/Rules.ts	96.80% <75.00%> (-0.97%)	⬇️
...iql/src/components/DocExplorer/MarkdownContent.tsx	100.00% <0.00%> (ø)
...ervice-interface/src/getAutocompleteSuggestions.ts	78.00% <0.00%> (ø)
...s/codemirror-graphql/src/utils/collectVariables.js
packages/codemirror-graphql/src/variables/lint.js
packages/codemirror-graphql/src/variables/mode.js
packages/graphiql-create-fetcher/src/lib.ts
packages/codemirror-graphql/src/utils/jsonParse.js
packages/codemirror-graphql/src/mode.js
... and 31 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 cb4debd...79382a5. Read the comment docs.

[acao]

awesome, thank you for starting on this!

i’ll have to take a closer look later. this parser is designed to handle inline and multiline graphql strings the same. in terms of tracking data within rules, steps are the best way to do that.

the change you made to runOnlineParser does seem out of place, however some change to the parser logic might be necessary for this, since no other rules really have to deal with this

[imolorhe]

62 files changed. Are all the changes in this PR related to fixing the block string parsing? It's hard to review the changes with such a large changeset. Might have something to do with the conflict in this branch too.

[acao]

@imolorhe it's my bad, I had to do some last few force pushes to main branch before we switched to changesets and were able to eliminate local publishing (and disable pushing to main at all, let alone force pushing)

@dwwoelfel I can take care of rebasing it for you if you like?

[imolorhe]

@acao Oh okay, that explains it.

[dwwoelfel]

I'll rebase against master and force push.

[changeset-bot]

🦋 Changeset detected

Latest commit: 79382a5

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

This PR includes changesets to release 6 packages

Name	Type
graphiql	Patch
codemirror-graphql	Patch
graphql-language-service-parser	Patch
graphql-language-service	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

[dwwoelfel]

The changes have been rebased.

[acao]

thank you for taking care of that so quickly @dwwoelfel ! hoping to take a look later

[acao]

@dwwoelfel on closer inspection, everything appears to be working as designed! excellent work

you are correct that this is a hack in terms of design, and a clever one. we can easily implement all of this in Rules.ts as you probably have guessed

1. add a MultilineString to LexRules, and take the regular expression for String, and separate it into two expressions

2. and you can add this below StringValue in ParseRules

MultilineStringValue: [p('"""'), 'StringValue', p('"""')],

3. and then go back up to Value in ParseRules and add this

  Value(token: Token) {
    switch (token.kind) {
    ...
	  case 'MultilineString':
	          return 'MultilineStringValue';
	  }
   }

[dwwoelfel]

@acao I don't think that approach can work for multiple lines in a block string. The approach works for rules like FragmentSpread because the multiple lines are separated by whitespace and onlineParser has special handling for whitespace that will advance the stream so you can get to the next line.

With a multiline block string, there is no way for the logic in Rules.js to consume the stream and advance to the next line.onlineParser tries to tokenize the string content as if it were the next part of the graphql document and you eventually end up with an invalid document.

Looking at the python codemirror mode, which also has multiline strings delimited by triple-quotes, they set a tokenizer in the state to handle multiple lines. Is that something you think could work here?

[acao]

@dwwoelfel that sounds like a great idea!

[ajbouh]

@dwwoelfel would you recommend your change as-is to fix block string parsing?

[dwwoelfel]

@ajbouh The patch was fully functional when I created the PR.

I think it's fine as-is, but an approach that uses a tokenizer is probably better for testing and maintainability.