Week 4: Homework #9
Conversation
There was a problem hiding this comment.
Overall pretty good! :) There is definitely a lot of space for improvements, but it's something we are about to address in future lessons (side effects most of all).
-
Personally, I don't like
WithConnectname. MaybeConnected${Name}would be better? -
And there is one misalignment in the naming of reducer vs. api methods - user vs. customer. I think it should be unified, so let's call our reducer a
customer. -
Let's add missing functionality - let's save into storage not just token, but the whole user. And also when initialization the store, let's check if user is in the storage
| import { setToken } from '../../utils/token' | ||
|
|
||
| export const getCustomerToken = async ({ username, password }) => { | ||
| const response = await fetch(`${config.apiUrl}/oauth/token`, { |
There was a problem hiding this comment.
@apedroferreira any chance to use there api/api-client.js?
There was a problem hiding this comment.
Sure! Do you think we should also use the API client for api/get-guest-token.js? Because for this I just followed the same approach as in that file.
Also what do you think about having separate files for get-guest-token and get-customer-token, even though technically they use the same endpoint? Is it the best approach?
I thought about having them in the same file but maybe that would be more confusing for the students.
There was a problem hiding this comment.
Also this would require some refactoring as currently the API client is for requests that need a token only, and getting the customer token doesn't need one.
So do you think we should refactor the way these methods are working in general so that the API client is used everywhere?
There was a problem hiding this comment.
Got it, now I understand your intention. All good! :)
The best approach would be probably a generic get-token function taking care of both :) But probably let's keep it as it is. @erikmajlath will refactor this together with refresh token handling
src/components/PrivateRoute/index.js
Outdated
| isAuthenticated: Object.keys(state.user).length !== 0, | ||
| }) | ||
|
|
||
| const WithConnect = connect(mapStateToProps)(PrivateRoute) |
There was a problem hiding this comment.
I got the intention, but naming is a bit confusing there... Not sure how to resolve it right now, but at least a comment here :D
There was a problem hiding this comment.
I also think this is confusing. 😅 I know that my opinion is not going to be popular but...
const WithConnect = connect(mapStateToProps)(Account)
export { WithConnect as Account }
...export default? 🙊🙂Less work, better readability.
export default connect(mapStateToProps)(Account)
I am using the normal export strategy in typescript because when I create files like:
models/Usermodels/RefreshToken
Then I can create models/index file:
export * from './RefreshToken'
export * from './User'
And then I can import anything from models by using:
import { User, RefreshToken } from '~/database/sql/models`
This can potentially save couple of lines of code. However, right now we are importing all pages separatelly anyway...
Maybe @varholak-peter can refactor this in his lesson?
There was a problem hiding this comment.
I don't like it either. When I moved from default exports to named exports (I will just leave it here for info - https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html) I haven't really used HOCs, because of the hooks.
However what we really don't want to do is to mix default exports with named. We need to keep it consistent.
src/components/Layout/index.js
Outdated
| isAuthenticated: Object.keys(state.user).length !== 0, | ||
| }) | ||
|
|
||
| export default connect( |
There was a problem hiding this comment.
This is gonna be a great candidate for a little compose (from redux) refactor for @varholak-peter 's lesson :)
| return ( | ||
| <Route | ||
| {...rest} | ||
| render={routeProps => { | ||
| if (isAuthenticated) { | ||
| return <Component {...routeProps} /> | ||
| } | ||
|
|
||
| return ( | ||
| <Redirect | ||
| to={{ |
There was a problem hiding this comment.
This should be changed to redirect to login
There was a problem hiding this comment.
Aaaand one more thing. Maybe let's extract route names into one file and use them as constants?
src/pages/LogIn/index.js
Outdated
| } | ||
|
|
||
| const mapDispatchToProps = { | ||
| login, |
There was a problem hiding this comment.
I am probably late to the party but for future reference, I am using mapDispatchToProps like this 🙂
const mapDispatchToProps = {
dispatchLogin: login,
}
Destructuring props will trigger eslint no-shadow. Here it can actually be confusing to differentiate betwen the two login functions.
There was a problem hiding this comment.
You are absolutely right.
What I usually do is to import all actions as follows:
import * as customerActions from '../store/customer/actions'
// and use it
const mapDispatchToProps = {
login: customerActions.login
}then when inspecting mapDispatchToProps I immediately know where it is coming from without looking at the import. I also don't favor tying names to the functionality, e.g. I don't want my component to know that this prop is coming from Redux 😄.
What do you think?
No description provided.