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.

Sign up for GitHub

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
merged 1 commit into from
Sep 28, 2016
Merged

Improved error handling for variables parsing #171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 34 additions & 25 deletions src/components/GraphiQL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,9 +478,14 @@ export class GraphiQL extends React.Component {
let jsonVariables = null;

try {
jsonVariables = JSON.parse(variables);
jsonVariables =
variables && variables.trim() !== '' ? JSON.parse(variables) : null;
} catch (error) {
jsonVariables = null;
throw new Error(`Variables are invalid JSON: ${error.message}.`);
Copy link
Contributor

@asiandrummer asiandrummer Sep 28, 2016 •
edited
Loading

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

}

if (typeof jsonVariables !== 'object') {
throw new Error('Variables are not a JSON object.');
}

const fetch = fetcher({
Expand Down Expand Up @@ -521,10 +526,7 @@ export class GraphiQL extends React.Component {

return subscription;
} else {
this.setState({
isWaitingForResponse: false,
response: 'Fetcher did not return Promise or Observable.'
});
throw new Error('Fetcher did not return Promise or Observable.');
}
}

Expand All @@ -549,27 +551,34 @@ export class GraphiQL extends React.Component {
}
}

// _fetchQuery may return a subscription.
const subscription = this._fetchQuery(
editedQuery,
variables,
operationName,
result => {
if (queryID === this._editorQueryID) {
this.setState({
isWaitingForResponse: false,
response: JSON.stringify(result, null, 2),
});
try {
// _fetchQuery may return a subscription.
const subscription = this._fetchQuery(
editedQuery,
variables,
operationName,
result => {
if (queryID === this._editorQueryID) {
this.setState({
isWaitingForResponse: false,
response: JSON.stringify(result, null, 2),
});
}
}
}
);
);

this.setState({
isWaitingForResponse: true,
response: null,
subscription,
operationName,
});
this.setState({
isWaitingForResponse: true,
response: null,
subscription,
operationName,
});
} catch (error) {
this.setState({
isWaitingForResponse: false,
response: error.message
});
}
}

handleStopQuery = () => {
Expand Down