-
Notifications
You must be signed in to change notification settings - Fork 730
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
fix: Update graphql-js error handling #3132
Conversation
-Updating the handling of graphql-js errors to check if source locations exist to determine how to process and display the error -This fixes the issue of reaching the max error limit of graphql-js
✅ Deploy Preview for apollo-ios-docs canceled.
|
@@ -305,8 +305,14 @@ public class ApolloCodegen { | |||
) | |||
|
|||
guard graphqlErrors.isEmpty else { | |||
let errorlines = graphqlErrors.flatMap({ $0.logLines }) | |||
CodegenLogger.log(String(describing: errorlines), logLevel: .error) | |||
let errorlines = graphqlErrors.flatMap({ |
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.
Thinking here is by making the sourceLocations
optional and therefore the logLines
optional, we can determine how to display a given error, so errors with sourceLocations
get displayed as they always have, but errors without sourceLocations
no longer cause a precondition failure and instead just have their name
and message
displayed to ensure the user always gets usable error information
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.
This is an awesome solution!
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.
Nice job! Just one thing to verify and this is good to go.
@@ -305,8 +305,14 @@ public class ApolloCodegen { | |||
) | |||
|
|||
guard graphqlErrors.isEmpty else { | |||
let errorlines = graphqlErrors.flatMap({ $0.logLines }) | |||
CodegenLogger.log(String(describing: errorlines), logLevel: .error) | |||
let errorlines = graphqlErrors.flatMap({ |
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.
This is an awesome solution!
if let logLines = $0.logLines { | ||
return logLines | ||
} else { | ||
return ["[Error] \($0.name ?? "unknown"): \($0.message ?? "")"] |
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.
I believe the [Error]
label is already prepended to the log by the CodegenLogger
. So putting it here might make it display it twice in the actual log.
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.
Good callout I'll remove that
-Updating the handling of graphql-js errors to check if source locations exist to determine how to process and display the error
-This fixes the issue of reaching the max error limit of graphql-js
Closes #3126