-
Notifications
You must be signed in to change notification settings - Fork 101
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
Rethrow human-readable error on schema introspection #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality LGTM.
Added some code review comments.
Thanks for the PR @BlackDahila :)
src/ui.js
Outdated
@@ -153,7 +153,23 @@ const executeQueryFromTerminalUI = async (queryOptions, successCb, errorCb) => | |||
headers, | |||
} = queryOptions; | |||
cli.action.start('Introspecting schema'); | |||
const schemaResponse = await query({endpoint: endpoint, query: introspectionQuery, headers: headers}); | |||
|
|||
const schemaResponse = await query({endpoint: endpoint, query: introspectionQuery, headers: headers}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better readable way for catching errors in the async await pattern is:
let schemaResponse;
try {
schemaResponse = await query({...})
} catch (e) {
// throw
}
const queryOptions = { | ||
query: queryString, | ||
endpoint: endpoint, | ||
endpoint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to do this assignment after you check if queryString
is null.
Assigning here and spreading later is just an extra step IMO.
try { | ||
schemaResponse = await query({endpoint: endpoint, query: introspectionQuery, headers: headers}); | ||
} catch (err) { | ||
if (err.message && err.message.startsWith('Network error: Unexpected token')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlackDahila you might want to check this condition. I tried running the command:
./bin/run http://localhost:random --introspect
It gave me this error:
Executing query... error
Error: ApolloError: Network error: request to http://localhost/:random failed, reason: connect ECONNREFUSED 127.0.0.1:80
at new ApolloError (/home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:124:32)
at /home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:1248:45
at /home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:1680:21
at Array.forEach (<anonymous>)
at /home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:1679:22
at Map.forEach (<anonymous>)
at QueryManager.broadcastQueries (/home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:1672:26)
at /home/wawhal/oss/graphql/graphqurl/node_modules/apollo-client/bundle.umd.js:1175:35
at processTicksAndRejections (internal/process/task_queues.js:85:5) {
graphQLErrors: [],
networkError: FetchError: request to http://localhost/:random failed, reason: connect ECONNREFUSED 127.0.0.1:80
at ClientRequest.<anonymous> (/home/wawhal/oss/graphql/graphqurl/node_modules/node-fetch/lib/index.js:1345:11)
at ClientRequest.emit (events.js:203:13)
at Socket.socketErrorListener (_http_client.js:399:9)
at Socket.emit (events.js:203:13)
at emitErrorNT (internal/streams/destroy.js:91:8)
at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
at processTicksAndRejections (internal/process/task_queues.js:77:11) {
message: 'request to http://localhost/:random failed, reason: connect ECONNREFUSED 127.0.0.1:80',
type: 'system',
errno: 'ECONNREFUSED',
code: 'ECONNREFUSED'
},
message: 'Network error: request to http://localhost/:random failed, reason: connect ECONNREFUSED 127.0.0.1:80',
extraInfo: undefined
}
Closes #18.
This PR shows a human-readable error when schema introspection fails on JSON parsing.
Before
After
Questions
I've rethrown the error in ui.js but I'm not 100% sure if it's the proper place.
My first attempt was to wrap lines 59 to 72 of command.js in a try-catch and doing it there. (That's why I moved
queryOptions
assignment a little bit higher. I've decided to leave it to make the code a little bit more DRY.)If there's anything that needs some more work just let me know :)