Skip to content

Conversation

@NickBolles
Copy link
Contributor

@NickBolles NickBolles commented Oct 3, 2019

This is a proposal for how to throw messages..

Before, data would be undefined and we would try to access the result field on data resulting in the following message:

  ● Accounts (e2e) › AccountsJS Client › when logged in › should login

    TypeError: Cannot read property 'tokens' of null

      at AccountsClient.<anonymous> (node_modules/@accounts/client/src/accounts-client.ts:97:35)
      at step (node_modules/tslib/tslib.js:136:27)
      at Object.next (node_modules/tslib/tslib.js:117:57)
      at fulfilled (node_modules/tslib/tslib.js:107:62)

After the change you get this message:

  ● Accounts (e2e) › AccountsJS Client › when logged in › should login

    GraphQLErrorList - 1 error in mutation: 
     mutation ($serviceName: String!, $params: AuthenticateParamsInput!) {
      authenticate(serviceName: $serviceName, params: $params) {
        sessionId
        tokens {
          refreshToken
          accessToken
        }
      }
    }
        - User not found

Close #823

@NickBolles
Copy link
Contributor Author

I also added a fix for the following error:

   GraphQLErrorList - 1 errors in mutation: 
     mutation impersonate($accessToken: String!, $username: String!) {
      impersonate(accessToken: $accessToken, username: $username) {
        authorized
        tokens {
          refreshToken
          accessToken
        }
        user {
          id
          email
          username
        }
      }
    }

        - Cannot query field "email" on type "User". Did you mean "emails"?

@pradel
Copy link
Member

pradel commented Oct 7, 2019

@NickBolles that's a good idea and a better behavior thanks for this 👍
I am just wondering if it would be better to use the default GraphqlError object?

@NickBolles
Copy link
Contributor Author

@pradel it looks like there's a potential to have multiple errors, and it's not possible to throw multiple errors, so I made a wrapper for it and let the consumer have access to the raw list of errors via the errors property. I'm up for figuring out a better way though!

@NickBolles
Copy link
Contributor Author

@pradel should we check if there's only one error and throw that error by itself, otherwise throw the errorList?

@pradel
Copy link
Member

pradel commented Oct 10, 2019

@NickBolles I think it's fine that way :) do you think you could add an example of how to handle the error in the graphql documentation? website/docs/transports/graphql.md

@NickBolles
Copy link
Contributor Author

sorry that took so long, I didn't see your response till now. Just added a little blurb about it. Let me know if you think it needs any changes.

While I was at it I updated the example to be more accurate (at least in my experience)

Copy link

@ducan-ne ducan-ne left a comment

Choose a reason for hiding this comment

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

cool

@pradel
Copy link
Member

pradel commented Dec 9, 2019

@NickBolles sorry I didn't get back to it earlier. Can you please resolve the conflicts you have so I can merge it? :)

@NickBolles
Copy link
Contributor Author

@pradel done!

@pradel
Copy link
Member

pradel commented Dec 30, 2019

@NickBolles looks like the tests are failing, can you please take a look?

@NickBolles
Copy link
Contributor Author

@pradel that should be fixed now. Sorry for the huge delay

@pradel pradel changed the title feat: throw a descriptive error (#823) feat(graphql-client): throw a descriptive error (#823) Feb 6, 2020
@pradel pradel changed the title feat(graphql-client): throw a descriptive error (#823) feat(graphql-client): throw a descriptive error Feb 6, 2020
@pradel pradel merged commit 74fe906 into accounts-js:master Feb 6, 2020
@pradel
Copy link
Member

pradel commented Feb 6, 2020

Thank you @NickBolles!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

graphql-client - throw error instead of generic error

3 participants