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

Don't swallow errors from Engine #705

Merged
merged 2 commits into from
Nov 14, 2018
Merged

Don't swallow errors from Engine #705

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be34bab
Don't swallow errors from Engine
trevor-scheer Nov 14, 2018
0578c0f
error.message is already a string, don't interpolate
trevor-scheer Nov 14, 2018
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
24 changes: 12 additions & 12 deletions packages/apollo-language-server/src/engine/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ export class ApolloEngineClient extends GraphQLDataSource {
);
}
// use error logger
// if (errors) {
// throw new Error(errors);
// }
if (errors) {
throw new Error(errors.map(error => error.message).join("\n"));
Copy link
Member

Choose a reason for hiding this comment

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

Can we DRY this up a bit by creating a helper? This seems like it's going to be a very common pattern that's going to perpetuate if we don't nip it in the bud right now.

At the very least, this might be a mapGraphQLErrorsToString(errors) (and be errors.map(error => error.message).join("\n")), but I'd even go so far to say we could have a assertNoGraphQLErrors(errors); function which would be all three lines here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally not opposed, and had considered this.

@jbaxleyiii mentioned to me that we're going to overhaul error handling, I'll leave it up to him if this is something I should take care of or not if this goes away soon anyway.

}
if (!data) {
throw new Error("Error in request from Engine");
}
Expand All @@ -151,9 +151,9 @@ export class ApolloEngineClient extends GraphQLDataSource {
);
}
// use error logger
// if (errors) {
// throw new Error(errors);
// }
if (errors) {
throw new Error(errors.map(error => error.message).join("\n"));
}
if (!data) {
throw new Error("Error in request from Engine");
}
Expand All @@ -170,9 +170,9 @@ export class ApolloEngineClient extends GraphQLDataSource {
);
}
// use error logger
// if (errors) {
// throw new Error(errors);
// }
if (errors) {
throw new Error(errors.map(error => error.message).join("\n"));
}
if (!data) {
throw new Error("Error in request from Engine");
}
Expand All @@ -190,9 +190,9 @@ export class ApolloEngineClient extends GraphQLDataSource {
);
}
// use error logger
// if (errors) {
// throw new Error(errors);
// }
if (errors) {
throw new Error(errors.map(error => error.message).join("\n"));
}
if (!data) {
throw new Error("Error in request from Engine");
}
Expand Down