Skip to content
This repository has been archived by the owner on Aug 5, 2022. It is now read-only.

Week 6 - Async and Errors #13

Closed
wants to merge 12 commits into from
Closed

Week 6 - Async and Errors #13

wants to merge 12 commits into from

Conversation

erikmajlath
Copy link

I need to finish error handling tomorrow but please feel free to provide any feedback :)

Copy link
Contributor

@dannytce dannytce left a comment

Choose a reason for hiding this comment

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

Overall great job! :) Looking forward to see the rest of implementation

src/components/Layout/index.js Outdated Show resolved Hide resolved
// If everything went fine just return the result
return response.json()
} catch (e) {
// TODO: redirect to login
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of guest token the redirect is actually not correct behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that in some cases this might lead to an infinite loop? 🤔Maybe when a user tries to log in with incorrect credentials? A possible fix would be something like this:

const makeRequest = (url, options, token, isRetried)

(and ignore the call if retried)

Copy link
Author

Choose a reason for hiding this comment

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

log in and singup are done without this api client so that should not be problem. We need to rethrow error so async validation can kick in later and handle just special cases (like second 401 with logout etc.)

src/api/customers/refresh-customer-token.js Outdated Show resolved Hide resolved
src/components/ErrorBoundary/index.js Outdated Show resolved Hide resolved
src/utils/refresh-token.js Outdated Show resolved Hide resolved
src/store/index.js Show resolved Hide resolved
src/App.js Outdated
@@ -1,16 +1,20 @@
import React from 'react'
import { Switch, Route, Redirect } from 'react-router-dom'
import { Provider } from 'react-redux'
import { ToastContainer, toast } from 'react-toastify'
import 'react-toastify/dist/ReactToastify.css'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's put that into globalStyles.js?

Copy link
Author

Choose a reason for hiding this comment

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

👍

src/api/api-client.js Show resolved Hide resolved
src/pages/Logout/index.js Show resolved Hide resolved
src/pages/Logout/index.js Show resolved Hide resolved
@dannytce
Copy link
Contributor

Closed in favor of #14

@dannytce dannytce closed this Apr 24, 2019
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.

None yet

3 participants