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

Uniform parsing of queries with short-hand syntax with regular queries #1094

Merged

Conversation

lebedev
Copy link
Contributor

@lebedev lebedev commented Nov 19, 2017

To date, parsing queries with short-hand syntax returns different results from parsing regular queries.

parse('query{a{b}}') returns

...
  name: undefined,
  variableDefinitions: [],
...

However, parse('{a{b}}') returns

...
  name: null,
  variableDefinitions: null,
...

image

This shouldn't be the case. This PR made queries with short-hand syntax to be parsed as regular.

This inconsistency indirectly causes this bug to happen in GraphiQL. Despite that issue being closed, using GraphiQL without express-graphql still makes that bug to happen (with null value, actually, but this doesn't change much).

Closes #729.

@IvanGoncharov
Copy link
Member

@angly-cat 😮 Wow, you solved the mystery of Unknown operation named \"undefined\". message.
👏

This PR made queries with short-hand syntax to be parsed as regular.

I think null is the correct value for missing name since it consistent with rest of AST format, e.g. alias, defaultValue, etc.

You can change line 285 to:

let name = null;

As a bonus it should fix broken tests 😉

@lebedev lebedev force-pushed the fix-parsing-of-queries-with-short-hand-syntax branch from d0c75b1 to bfef7b3 Compare November 22, 2017 18:38
by making the parser return the same result as with short-hand syntax
queries
@lebedev lebedev force-pushed the fix-parsing-of-queries-with-short-hand-syntax branch from bfef7b3 to ceff6f7 Compare November 22, 2017 18:42
@lebedev
Copy link
Contributor Author

lebedev commented Nov 22, 2017

Wow, you solved the mystery of Unknown operation named \"undefined\". message.
👏

Thanks for your kind words.

I think null is the correct value for missing name since it consistent with rest of AST format, e.g. alias, defaultValue, etc.

I agree.

However, all of my attempts to change output of short-hand syntax query parsing caused a test to fail. (OMG it was quite hard to make tests work on Windows) According to these mocha tests, short-hand syntax queries were parsed correctly this whole time and, therefore, the regular queries were parsed incorrectly instead! So I made a couple of changes to uniform the results of parsing short-hand syntax queries with regular, and now all tests are passed.

@lebedev lebedev changed the title Fix parsing of queries with short-hand syntax Uniform parsing of queries with short-hand syntax with regular queries Nov 22, 2017
@IvanGoncharov
Copy link
Member

@angly-cat Great 👍
It would be great if you add test case to cover parse('query{a{b}}') to prevent future regressions.

@lebedev
Copy link
Contributor Author

lebedev commented Nov 23, 2017

@IvanGoncharov the idea about adding a test was great, so, I added it.

@IvanGoncharov
Copy link
Member

@angly-cat Great 👍
Now we need to wait for someone with commit rights to merge it.

@leebyron
Copy link
Contributor

Nice work!

I think since other array-parsing operations default to empty arrays instead of null, that this should follow that convention, so I made some updates.

@leebyron leebyron merged commit 7a7ffe4 into graphql:master Nov 30, 2017
@lebedev
Copy link
Contributor Author

lebedev commented Nov 30, 2017

I think since other array-parsing operations default to empty arrays instead of null, that this should follow that convention, so I made some updates.

Agreed. Thanks, @leebyron.

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.

Some AST consistency questions
4 participants