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

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

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

dwwoelfel
Copy link
Contributor

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
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

acao commented Feb 1, 2021

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

@acao acao force-pushed the main branch 6 times, most recently from 4fa9ca3 to 1c11938 Compare February 1, 2021 18:22
@acao acao requested a review from imolorhe February 2, 2021 20:16
@imolorhe
Copy link
Contributor

imolorhe commented Feb 2, 2021

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
Copy link
Member

acao commented Feb 2, 2021

@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
Copy link
Contributor

imolorhe commented Feb 2, 2021

@acao Oh okay, that explains it.

@dwwoelfel
Copy link
Contributor Author

I'll rebase against master and force push.

@changeset-bot
Copy link

changeset-bot bot commented Feb 2, 2021

🦋 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
Copy link
Contributor Author

The changes have been rebased.

@acao
Copy link
Member

acao commented Feb 3, 2021

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

@acao
Copy link
Member

acao commented Feb 4, 2021

@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('"""')],
  1. and then go back up to Value in ParseRules and add this
  Value(token: Token) {
    switch (token.kind) {
    ...
	  case 'MultilineString':
	          return 'MultilineStringValue';
	  }
   }

@dwwoelfel
Copy link
Contributor Author

dwwoelfel commented Feb 16, 2021

@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
Copy link
Member

acao commented Apr 18, 2021

@dwwoelfel that sounds like a great idea!

@ajbouh
Copy link

ajbouh commented Jun 29, 2021

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

@dwwoelfel
Copy link
Contributor Author

@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.

@acao acao merged commit 75dbb0b into graphql:main Oct 29, 2021
@github-actions github-actions bot mentioned this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants