Move logic to redux #507

mradenovic · 2021-01-31T04:12:01Z

Pros:

Strongly recomended
Easier testing
Easier mocking of libraries and use of storybook
More flexible notification handling

Resolves #517
Resolves #518

daltonfury42 · 2021-01-31T04:19:21Z

Wonderful, I always wanted to move to using thunk and actions, but could never get it right. Redux toolkit way of doing this was a bit confusing to me. Let me study your PR.

Pros: - [Strongly recomended](https://redux.js.org/style-guide/style-guide#put-as-much-logic-as-possible-in-reducers) - Easier testing - Easier mocking of libraries and use of storybook - More flexible notification handling

mradenovic · 2021-01-31T04:59:55Z

redux-toolkit was confusing for me too. Actually, it is very elegant. What makes things complicated is that `auth0' works only in components.

My idea is to pass auth0 to any async thunk. To suppress serializableCheck warnings, it must be ignored. Convention to make things clear would be:

const someAsyncAction = createAsyncThunk('action/requestStatus', async (arg) => {
  const { auth, payload } = arg;
  const { id } = payload
  const response = await doSomeethingById(auth, id);
  return response;
});

/**
 * Dispatching async actions
 *
 * When dispatching async actions with auth0,
 * wrap the `payload` within `arg` parameter of the `payloadCreator` function.
 *
 * Example:
 */

const auth = useAuth()
const payload = { id: '0' }
const arg = {
  auth: auth,
  payload: payload
}
dispatch(someAsyncAction(arg))

maaverik · 2021-01-31T12:38:26Z

I've never worked with thunk, so I'll hold off on reviewing till I get a good understanding of it. But essentially, the idea here is to abstract away both the authentication and API calls to reducers, right @mradenovic ?

mradenovic · 2021-01-31T14:30:54Z

I've never worked with thunk, so I'll hold off on reviewing till I get a good understanding of it. But essentially, the idea here is to abstract away both the authentication and API calls to reducers, right @mradenovic ?

Yes. It is easier for maintenance, and if decided to change something in the logic (i.e. replace auth0 with Firebase auth ), it can be done in one place.

simplq/src/components/pages/Home/MyQueues.jsx

simplq/src/store/queuesSlice.js

daltonfury42 · 2021-02-01T16:32:36Z

We should centralise this into redux. Our components would then look so much more clean. Can you also add one more where there will be a payload?

mradenovic · 2021-02-01T16:47:57Z

We should centralise this into redux. Our components would then look so much more clean. Can you also add one more where there will be a payload?

I am working on next itteration. I managed to completely move auth to async thunk with hook. It has some quirks though. I also discovered some bug. It looks like current delete functionality doesn't update myQueues. It worked before #504 was applied, but that is because that bug was constantly fetchin data from the server.

daltonfury42 · 2021-02-01T17:00:33Z

I managed to completely move auth to async thunk with hook

Interesting, how did you do that? I did find somewhere a custom middleware+local storage for token solution, but didn't like it.

Yes, I also noticed this. As a side effect of #504 it was getting updated before. But this issue would go away when we move to redux and have the delete queue action that would update the global store, I guess?

mradenovic · 2021-02-01T17:18:42Z

One more curriosity here: The server does not check if user is authenticated. If I send a request from client as anonyous, I get all the queues (300+). Than I can delete any of the queues even if I am not the owner.

// api/axios-alt.js

// to test, change line 16 declaration

  const accessToken = false //auth.isAuthenticated
    ? await auth.getAccessTokenSilently({ audience: 'https://devbackend.simplq.me/v1' })
    : 'anonymous';

daltonfury42 · 2021-02-01T17:26:15Z

Yes, this in in my list. I am thinking of doing this:

When a user is not logged in, we create a UUID and store it in the local storage. Instead of Bearer anonymous, we would send this ID in case of unauthenticated users. This ID would be used to identify the resources created by users who have not signed in.

Then post sign in, we call a backend API that would transfer ownerships from this ID to the signed in user, if such an ID exists in local storage.

What do you think? Ideally there should be some anonymous login, which would generate short lived tokens, but this setup looks good enough for me.

mradenovic · 2021-02-01T17:40:24Z

What do you think? Ideally there should be some anonymous login, which would generate short lived tokens, but this setup looks good enough for me.

I don't know much about handling users on the backend. I usually use ready made solutions when I need one 🤣 Firebase auth has all of that solved and that was what I used in the past for prototyping.

mradenovic · 2021-02-02T20:38:43Z

simplq/src/components/pages/Home/MyQueues.jsx

-  const { isAuthenticated } = useAuth0();
+  const dispatch = useDispatch();
+  const queues = useSelector(selectQueues);
+  const fetchQueues = useCallback(useFetchQueues(), []);


Use useCallback() only if the action is going to be dispatched in useEffect().

simplq/src/components/pages/Home/MyQueues.jsx

daltonfury42

Still understanding the change, thanks!

simplq/src/api/auth.js

simplq/src/api/requestFactory/index.js

simplq/src/api/requestFactory/queue.js

simplq/src/components/pages/Home/MyQueues.jsx

simplq/src/components/pages/Status/QueueDetails.jsx

simplq/src/store/appSlice.js

simplq/src/store/queues/queuesSlice.js

simplq/src/store/queues/queuesAsyncActions.js

The user is already on Home page. There is no need to redirect.

daltonfury42 · 2021-02-04T15:33:59Z

simplq/src/store/queues/queuesAsyncActions.js

+ * const fetchQueues = useFetchQueues()
+ */
+const useFetchQueues = () => {
+  const auth = useAuth();


this is pretty smart ✌️

daltonfury42 · 2021-02-04T15:42:54Z

simplq/src/store/queues/queuesAsyncActions.js

+    if (!auth || !auth.isAuthenticated) {
+      return { queues: [] };
+    }


Do we need !auth here? We should also add auth. isLoading here, I think.

Also leave this comment:

// TODO: Remove `!auth.isAuthenticated` after resolving https://github.com/SimplQ/simplQ-frontend/issues/477

We definitely do not need !auth, it is guaranteed to be there by the hook. There was a lot of copy-paste happening so there will be a lot to remove 😊 I haven't explored what exactly should happen when not authenticated, so that should be implemented later.

daltonfury42 · 2021-02-04T15:43:32Z

simplq/src/store/queues/queuesAsyncActions.js

+
+ * @see ./queuesSlice
+ *
+ * @see https://redux-toolkit.js.org/api/createAsyncThunk#return-value


I'll have to read this 😄

@daltonfury42 where can I find deployed PR?

I'm sorry, I didn't understand the last one.

@mradenovic We generate temporary preview deployments. This can be triggered by putting any label on the PR (You can see a sample here)

But due to some security limitations, this is not possible on PRs from forks.

OK, I'll push to repo to see how it works.

daltonfury42

Can't wait to merge this. It will be massive. 💪

For me weekend is here, I will migrate remaining components as soon as this PR goes in.

simplq/src/api/requestFactory/queue.js

daltonfury42 · 2021-02-05T16:55:28Z

simplq/src/store/queues/queuesSlice.js

@@ -0,0 +1,26 @@
+import { createSlice } from '@reduxjs/toolkit';


We should rename the file to userQueusSlice, and correspondingly make changes in lines 4,5,24 and 26.. and also outside usages.

simplq/src/store/asyncActions/getUserQueues.js

simplq/src/store/asyncActions/getQueueStatusByName.js

simplq/src/store/asyncActions/getQueueStatus.js

daltonfury42 · 2021-02-05T17:07:23Z

simplq/src/store/asyncActions/getQueueStatusByName.js

+        history.push(`/pageNotFound/queueName=${queueName}`);
+        return rejectWithValue({ message: `Queue ${queueName} does not exist, try again...` });


simplq/src/store/asyncActions/deleteQueue.js

daltonfury42 · 2021-02-05T17:12:48Z

simplq/src/components/pages/Status/QueueDetails.jsx

+      loading={!queueStatus}
    >
-      <QueueStats queueStatus={queueStatusResponse} />
+      <QueueStats queueStatus={queueStatus} />


This won't work, we would have to check for empty object. 🤔

An isLoading flag might be better, as you were implementing before?

These kind of glitches we can fix in a follow up PR as well.

All pending', fulfilled, and rejected` should be handled in reducers. It can be done later.

code looks much cleaner now

mradenovic · 2021-02-05T23:36:39Z

I suggest we continue the conversation at #522.

Move logic to redux

a529132

Pros: - [Strongly recomended](https://redux.js.org/style-guide/style-guide#put-as-much-logic-as-possible-in-reducers) - Easier testing - Easier mocking of libraries and use of storybook - More flexible notification handling

mradenovic force-pushed the move-logic-to-redux branch from 5784fd3 to a529132 Compare January 31, 2021 04:21

Move auth to async action hook

d3180fc

daltonfury42 reviewed Feb 1, 2021

View reviewed changes

simplq/src/components/pages/Home/MyQueues.jsx Outdated Show resolved Hide resolved

simplq/src/store/queuesSlice.js Outdated Show resolved Hide resolved

Queue handleDelete() with dispatch()

3d3ea48

daltonfury42 mentioned this pull request Feb 2, 2021

added share option #510

Merged

mradenovic added 5 commits February 2, 2021 12:48

Update state on queue delete

939a03a

Merge branch 'master' into rb

ea73ba5

Remove redundant parameter

3b21f91

Use call back for useEfffect() dependency

119e480

Merge branch 'master' into move-logic-to-redux

cd6d3d5

mradenovic commented Feb 2, 2021

View reviewed changes

simplq/src/components/pages/Home/MyQueues.jsx Outdated Show resolved Hide resolved

mradenovic marked this pull request as ready for review February 2, 2021 21:30

mradenovic requested a review from maaverik as a code owner February 2, 2021 21:30

Cosolidate api files

30f76a5

mradenovic marked this pull request as draft February 3, 2021 12:23

Refactor QueueDetails component

5845eb0

daltonfury42 reviewed Feb 3, 2021

View reviewed changes

Fix typos in api/

29e4381

mradenovic added 3 commits February 3, 2021 15:13

Remove redundant request creator

5de77df

Remove redundant reference

99f5374

Remove redundant history manipulation

058b6e4

The user is already on Home page. There is no need to redirect.

daltonfury42 reviewed Feb 4, 2021

View reviewed changes

mradenovic added 14 commits February 4, 2021 17:07

Fix: Unwrap Result before using it

447dcb9

Add getQueueStatusByName request creator

6462333

Add qetQueueStatusByName async action

3bad0c5

Update Join page with dispatch

76db038

Refactor folder sturcture

835acdd

Merge branch 'master' into move-logic-to-redux

c6a9b2c

Add selectedQueue slice

e1a0d8f

Fix action type to match

515f5f1

Connect Join page with redux store

2094c4f

Redirect from async action on Join error

cc81ab8

Update requestFactory docs

32682f2

Use constant for typePrefix in async actions

d549470

Connect QueueDetails to redux store

fc1d2b5

Rename selectedQueueSlice --> queueStatusSlice

f4b6038

mradenovic added the discuss label Feb 5, 2021

daltonfury42 reviewed Feb 5, 2021

View reviewed changes

mradenovic closed this Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move logic to redux #507

Move logic to redux #507

mradenovic commented Jan 31, 2021 •

edited

Loading

daltonfury42 commented Jan 31, 2021

mradenovic commented Jan 31, 2021

maaverik commented Jan 31, 2021

mradenovic commented Jan 31, 2021

daltonfury42 commented Feb 1, 2021

mradenovic commented Feb 1, 2021

daltonfury42 commented Feb 1, 2021 •

edited

Loading

mradenovic commented Feb 1, 2021

daltonfury42 commented Feb 1, 2021

mradenovic commented Feb 1, 2021

mradenovic Feb 2, 2021

daltonfury42 left a comment

daltonfury42 Feb 4, 2021

daltonfury42 Feb 4, 2021

mradenovic Feb 5, 2021

daltonfury42 Feb 4, 2021

Vishv07 Feb 5, 2021

mradenovic Feb 5, 2021

daltonfury42 Feb 5, 2021

mradenovic Feb 5, 2021

daltonfury42 left a comment

daltonfury42 Feb 5, 2021

daltonfury42 Feb 5, 2021

daltonfury42 Feb 5, 2021

mradenovic Feb 5, 2021

Vishv07 Feb 6, 2021

mradenovic commented Feb 5, 2021

		@@ -0,0 +1,26 @@
		import { createSlice } from '@reduxjs/toolkit';

		history.push(`/pageNotFound/queueName=${queueName}`);
		return rejectWithValue({ message: `Queue ${queueName} does not exist, try again...` });

Move logic to redux #507

Move logic to redux #507

Conversation

mradenovic commented Jan 31, 2021 • edited Loading

daltonfury42 commented Jan 31, 2021

mradenovic commented Jan 31, 2021

maaverik commented Jan 31, 2021

mradenovic commented Jan 31, 2021

daltonfury42 commented Feb 1, 2021

mradenovic commented Feb 1, 2021

daltonfury42 commented Feb 1, 2021 • edited Loading

mradenovic commented Feb 1, 2021

daltonfury42 commented Feb 1, 2021

mradenovic commented Feb 1, 2021

Choose a reason for hiding this comment

daltonfury42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daltonfury42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mradenovic commented Feb 5, 2021

mradenovic commented Jan 31, 2021 •

edited

Loading

daltonfury42 commented Feb 1, 2021 •

edited

Loading