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

Improved error handling for variables parsing #171

Merged

Conversation

OlegIlyenko
Copy link
Contributor

As discussed in #168, this PR improves error reporting for variable parsing:

variables-parsing-error

Here I handle 3 general cases:

  • variables are null/undefined or an empty string - variables set to null
  • variables string contains malformed JSON - result pane will show a detailed error message
  • variables JSON is valid, but does not contain a JSON object - result pane will show an error message: "Variables are not a JSON object."

Along the way I also noticed that "Fetcher did not return Promise or Observable." error was not reported correctly. Existing code was setting the state, but after _fetchQuery is finished, the state is immediately reset once again. It works for all other scenarios because they all set the state in an async callbacks, but it's not that case with "Fetcher did not return Promise or Observable." error. In order to solve the issue, I refactored this bit with throw/try/catch.

@ghost ghost added the CLA Signed label Sep 24, 2016
@OlegIlyenko OlegIlyenko force-pushed the improved-error-handling-for-variavles branch from e1c6b17 to 90cfe0e Compare September 24, 2016 13:51
} catch (error) {
jsonVariables = null;
throw new Error(`Variables are invalid JSON: ${error.message}.`);
Copy link
Contributor

@asiandrummer asiandrummer Sep 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of showing a string, should we create a JSON error format for these errors, like the ones that server gives back?

{
  "errors": [
    {
      "message": "Variables are invalid JSON."
    }
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer to keep it as a simple string for these 2 reasons:

  • As far as I can tell, other parts of GraphiQL also follow this pattern and render errors as a string ("Fetcher did not return Promise or Observable." is an example of it). But maybe I missed something?
  • Rendering this error as a JSON may case a confusion because it looks exactly like a server response, even though no request was sent to the server in case of malformed variables.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can agree with that ;p

@asiandrummer
Copy link
Contributor

@OlegIlyenko thanks for taking care of this ;)

@ghost ghost added the CLA Signed label Sep 28, 2016
@asiandrummer asiandrummer merged commit 94af6ed into graphql:master Sep 28, 2016
acao pushed a commit to acao/graphiql that referenced this pull request Jun 5, 2019
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.

3 participants