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

Remove "[GraphQL|Network] error" from error messages #3892

Merged
merged 1 commit into from
Apr 25, 2020
Merged

Conversation

lorensr
Copy link
Contributor

@lorensr lorensr commented Sep 7, 2018

Implements FR: apollographql/apollo-feature-requests#46

For consistency, strips the prefix from both GraphQL and Network errors. See "Stripping prefix" in #1812 (comment)

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Hi @lorensr - it looks like you might have prettier (or similar) rules in place that conflict with the defaults used by Apollo Client. Can you update this PR to revert all styling back to the defaults used by AC? For example, semi-colons are being stripped when we'd like them to be preserved. Thanks!

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #3892 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3892   +/-   ##
=======================================
  Coverage   89.14%   89.14%           
=======================================
  Files          43       43           
  Lines        2358     2358           
  Branches      575      552   -23     
=======================================
  Hits         2102     2102           
  Misses        240      240           
  Partials       16       16
Impacted Files Coverage Δ
packages/apollo-client/src/errors/ApolloError.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5eaf2d...de577c8. Read the comment docs.

@lorensr
Copy link
Contributor Author

lorensr commented Sep 26, 2018

Oops, thanks—the commit hook got me! It's working for me after adding --no-config, if that's okay?

@benjamn
Copy link
Member

benjamn commented Oct 8, 2018

@lorensr @hwillson Honestly I think we should remove the auto-git add prettier behavior here. It totally undermines the expectation that git should only commit code that you git added.

@hwillson
Copy link
Member

hwillson commented Oct 8, 2018

@benjamn Totally agree - this has thrown me off multiple times. Noted and I'll take care of it shortly (unless you beat me to it). Thanks!

@hwillson
Copy link
Member

Just re-visiting this a bit @benjamn; if we drop the git add, then we're no longer enforcing staged file prettier cleanup. We're currently doing this on all of our main repos, so this might be a larger discussion. For now though I think @lorensr's suggestion of making sure the pre-commit prettier hook doesn't use a local config if one exists (since we don't want it to), makes sense.

@hwillson hwillson dismissed their stale review October 12, 2018 17:28

Requested changes were made.

@abernix
Copy link
Member

abernix commented Nov 16, 2018

Just as a flag, Node.js usually flags changes to error message properties as breaking changes which must be included in semver-major releases. I don't know if we need to subscribe to that, but it's worth mentioning.

@settings settings bot removed the in progress label Dec 12, 2018
@lorensr lorensr requested a review from benjamn as a code owner December 13, 2018 00:21
@hwillson hwillson added this to the Release 3.0 milestone Jan 2, 2019
@hwillson
Copy link
Member

hwillson commented Jan 2, 2019

That's a really great point @abernix. People could be relying on the error message prefix to drive application logic. I think it makes sense to follow Node's lead here, so I'll add this to the Release 3.0 milestone. Thanks!

@ScreamZ
Copy link

ScreamZ commented Jun 6, 2019

Will this will be merged soon?

@mattdavenport
Copy link

Any updates here? This would be really valuable in displaying errors much more efficiently. Thanks!

@ScreamZ
Copy link

ScreamZ commented Oct 18, 2019

@benjamn @hwillson

@austinChappell
Copy link

Any updates on this?

`ApolloError` already provides a way to differentiate between
GraphQL and network errors (via its public `graphQLErrors` and
`networkError` properties), so let's get the extra details out
of the error message string itself.
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for kickstarting this @lorensr!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants