Skip to content

Conversation

@rymaju
Copy link
Member

@rymaju rymaju commented Mar 23, 2021

Why

When logging out and deleting the user we need to clear tokens in redux, we currently dont have the ability to do this with an action, so this PR aims to add that feature.

Copy link
Member

@jackblanc jackblanc left a comment

Choose a reason for hiding this comment

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

This looks good, but considering this is a core util, I think we need a unit test. There's a few things that shouldn't be too complicated to test here. First, we want to expect that genericAsyncActions now additionally returns the notStarted actions for all AsyncRequests. Second, we want to expect that calling the reducer with this generated action will result in the field being returned as an AsyncRequestNotStarted. Let me know if you want help with this. Hey, maybe coveralls will go up for once!

@rymaju
Copy link
Member Author

rymaju commented Mar 23, 2021

Imagine writing tests 😅

@rymaju rymaju requested a review from jackblanc March 23, 2021 19:49
const action = authenticateUser.notStarted();
const authenticatedState: UserAuthenticationReducerState = {
...initialUserState,
tokens: AsyncRequestCompleted<TokenPayload, void>(payload),
Copy link
Member

Choose a reason for hiding this comment

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

I think technically it's an any error right?

Copy link
Member

@jackblanc jackblanc left a comment

Choose a reason for hiding this comment

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

🚢, not sure if change void to any matters at all

@rymaju
Copy link
Member Author

rymaju commented Mar 23, 2021

It passes so hey lgtm

@rymaju rymaju merged commit 6e47a36 into master Mar 23, 2021
@rymaju rymaju deleted the ryanjung_clear_tokens branch March 23, 2021 19:52
@rymaju rymaju mentioned this pull request Mar 23, 2021
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.

3 participants