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

Improve Api Error Handling #22

Closed
jamsinclair opened this issue Jun 9, 2019 · 4 comments

Comments

@jamsinclair
Copy link

jamsinclair commented Jun 9, 2019

@idanlo Thanks for the great work on this project!

Using the library I noticed it does not set an error state when the API returns a non-200 status code. The error JSON data is returned in the data field.

Reproduction:

import { SpotifyApiContext, Artist } from 'react-spotify-api';

<SpotifyApiContext.Provider value="bad_access_token">
  <Artist id="6eUKZXaKkcviH0Ku9w2n3V">
    {(artist, loading, error) => (
      <div>
        <p>Artist: {JSON.stringify(artist)}</p>
        <p>Error: {JSON.stringify(error)}</p>
        <p>Loading: {JSON.stringify(loading)}</p>
      </div>
    )}
  </Artist>
</SpotifyApiContext.Provider>

Result:

Artist: {"error":{"status":401,"message":"Invalid access token"}}

Error: false

Loading: false

I would like to propose setting an error object when encountered with the API or at the very least set to true. Do you have any thoughts on this? Happy to help 😄

@idanlo

This comment has been minimized.

Copy link
Owner

idanlo commented Jun 9, 2019

@jamsinclair cfc85f4 this commit should fix it. I just released version 2.0.1 with this fix.
Please update if it works as it should

@jamsinclair

This comment has been minimized.

Copy link
Author

jamsinclair commented Jun 9, 2019

Thanks @idanlo, beat me to it! That solution does work for me.

I've made a PR over in #23 that also sets the error object in the state. I'm happy either way.

@idanlo

This comment has been minimized.

Copy link
Owner

idanlo commented Jun 9, 2019

Thanks @jamsinclair I actually already made some changes but you have some interesting stuff with the res.status which I will implement. Also the testing you wrote - 👍

idanlo added a commit that referenced this issue Jun 9, 2019
idanlo added a commit that referenced this issue Jun 9, 2019
…ct/null and not a boolean value
@idanlo

This comment has been minimized.

Copy link
Owner

idanlo commented Jun 9, 2019

Thank you @jamsinclair, this is now fixed and all tests are passing.
I will close your pull request

@idanlo idanlo closed this Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.