Graphiql: regressions in variable validation

[tnrich]

Hey there,

First off, thanks for making Graphiql! It's great!

I am getting an error similar to the one described in this issue: https://github.com/graphql/graphiql/issues/1308

Here's the query I have in the inspector:

{
  sequences{
    pageSize
    pageNumber
    results {
      size
      sequenceFeatures {
        start
        end
        name
      }
    }
  }
}

{
  seq
}

And here are the errors:

I believe this is because the logic passing the highlightNode into the getLocation function is faulty:

      const highlightNode =
      node.kind !== 'Variable' && 'name' in node
        ? node.name
        : 'variable' in node
        ? node.variable
        : node;

As you can see in the above image, node.name is undefined yet 'name' in node still === true. This causes undefined to be passed to getLocation on line 137: const highlightLoc = getLocation(highlightNode);

In the past that code used to look like:

  return error.nodes.map(node => {
    const highlightNode =
      node.kind !== 'Variable' && node.name
        ? node.name
        : node.variable
        ? node.variable
        : node;

But got converted in this PR https://github.com/graphql/graphiql/pull/957 to look like:

I am going to make a PR to fix this specific issue where the xxx in xxx syntax is being used incorrectly. I think it someone on the graphiql team should look at all the places that got changed to use the xxx in xxx syntax that might now break.

Thanks!

P.S. In case it wasn't clearn, once I made the change I described above locally, the error I was seeing went away and graphiql resumed working properly again

[acao]

@tnrich awesome find, thank you!

can you write some tests to show where it's broken, by chance? all the tests are passing, but I don't recall offhand whether these lines have coverage

we needed to make these changes when migrating from flow javascript to typescript

it seems you want to use && obj.value !== undefined instead of && obj.value

[tnrich]

@acao hmmmm it looks like someone added a check 4 months ago that causes this issue to no longer show up on master, however it still is an issue on 0.17.5:

if (highlightNode) {

that check makes this no longer get hit.

With that if-statement commented out, I do see the error in the test I wrote:

I think that the current code is better but still has issues because the node that should be highlighted is now just ignored.

I've updated my PR (https://github.com/graphql/graphiql/pull/1566) changing the code slightly and adding a test to make sure that errors where node.name = undefined will still show up.

[tnrich]

@acao could you advise on what version I should use to get around this error in the meantime? I believe v0.17.5 is the highest non-beta version but it doesn't have the code that's on master. I couldn't tell which branch 0.17.5 is cut from? Thanks!

[acao]

every release is cut from master branch

try out the alphas? https://www.npmjs.com/package/graphiql

easiest way to get at the last pre-release is npm install graphiql@next or yarn add graphiql@next

[acao]

thanks @tnrich ! I think we're good to go on this one, let me know if you notice any other issues.

[acao]

after testing, it seems variables in lists are still having trouble, and i’m pretty sure this validation issue goes all the way down to graphql-js itself?

[tnrich]

@acao can you elaborate more on the issue you're seeing? I think my PR fixed the issue I was encountering

[acao]

@tnrich yes, i thought I saw another issue but it's actually expected behavior! thanks for this, now that 1.0.0 is released, are we good to consider this resolved?

[tnrich]

yep thanks!