-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
regressions in variable validation #1565
Comments
@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 |
@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 (#1566) changing the code slightly and adding a test to make sure that errors where |
@acao could you advise on what version I should use to get around this error in the meantime? I believe |
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 |
thanks @tnrich ! I think we're good to go on this one, let me know if you notice any other issues. |
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? |
@acao can you elaborate more on the issue you're seeing? I think my PR fixed the issue I was encountering |
@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? |
yep thanks! |
Hey there,
First off, thanks for making Graphiql! It's great!
I am getting an error similar to the one described in this issue: #1308
Here's the query I have in the inspector:
And here are the errors:
I believe this is because the logic passing the
highlightNode
into thegetLocation
function is faulty:As you can see in the above image,
node.name
is undefined yet'name' in node
still === true. This causesundefined
to be passed togetLocation
on line 137:const highlightLoc = getLocation(highlightNode);
In the past that code used to look like:
But got converted in this PR #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 thexxx 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
The text was updated successfully, but these errors were encountered: