Redux cleaning #550

yesean · 2022-03-11T01:41:32Z

This PR is a refactor of our redux usage, most notably the help of redux-toolkit. The main goal was to simplify the code and reduce the amount of boilerplate brought on by redux. This is achieved in multiple ways:

Using toolkit's createSlice and createAsyncThunk to define redux reducers and actions. These helper functions reduce a lot of redux code (with the help of immutable-js and typescript) which allows all of the redux to be colocated into a single *Slice.ts file.
Replacing the use of the connect HOC with the useDispatch and useSelector hooks which effectively remove the need for most container components.
General purpose refactoring to keep the code (including some redux-adjacent stuff) as DRY but simple as possible.

Addtional notes:

*{Reducer,Actions,Types}.ts files have been replaced with *Slice.ts and a utils.ts. the slice file holds all of the redux logic and the utils file holds the non-redux logic (fetching, notifying, etc). generally, the functions in utils are imported into the slice to provide any needed redux for the utility. this was done to decouple the redux and non-redux logic as much as possible.
the redux_reducers.ts, combineReducers, createStore setup has been replaced with the simpler redux/store.ts, configureStore (the toolkit recommended way)
redux-toolkit provides excellent typescript support for the store and actions which was previously severely lacking. I tried to type as much as I could but some parts of the store (in particular, the event slice) are poorly typed (dynamically changing slice structures) so it was hard to infer exactly what fields would be in each slice. In any case, the current type safety is far better than before (some fields may still be missing though).
I needed the Awaited utility type which was introduced in Typescript 4.5 so I upgraded the typescript package to latest. Unfortunately, typescript changed their AST in 4.0 but create-react-app uses an older, incompatible version of typescript-eslint/parser so I used selective dependency resolutions (resolutions in package.json) to force the new version of the eslint parser. Nevertheless, everything is working fine now, though this change may be better suited for another PR?
I was pretty aggresive with the refactor and I did change some of the business logic and I'm not too familiar all of the inner workings of the portal so I may have made some wrong assumptions about various processes (auth, checkin, etc). I haven't tested it too extensively, for the most part, it works but I'm hoping this PR can not only serve as a code review but an open call for testing.

* Separate History from Redux * Update References to History * Remove connected-react-router * Update Location Usage Co-authored-by: Matei-Alexandru Gardus <[email protected]> Co-authored-by: Steven Steiner <[email protected]>

…ncy)

StormFireFox1 · 2022-03-13T01:34:21Z

This is some great work, @yesean, well done!

I've looked over your PR at a first glance, and overall, it's good! I haven't reviewed the business logic you've changed yet, but I'll probably do that after finals week. I think there's a couple questions that came up while I looked over it we might want to discuss:

Currently, most of each feature's utils.ts is effectively the API calls, whereas conventionally utilities were simply local functions we used to simplify business logic. Currently, @steets250 was moving API calls under an API folder of sorts, maybe we wanna do something similar here.
Also, on a more fun note, like you said, I don't think we need containers anymore! It's kinda hard to truly move away from them, considering how prominent they are all around the code, so I suggest we rename all our containers to pages, which makes more sense as to what purpose they serve; representing each page from our portal.

What do y'all think of this?

steets250 · 2022-03-13T01:53:48Z

Ah sorry I've been MIA for the past week(s) about this, you can completely ignore the api-updates branch, that was my wild adventure of "oh, now that this is fixed I can also fix this", which I'm still in the process of splitting up into more reasonably sized PRs. Tomorrow I'll take review this, and hopefully the switch to separate api functions can just happen in place once these changes are either merged in, or a PR can be opened against this branch instead if we want to hold off on merging until everything is done.

steets250 · 2022-03-14T04:38:10Z

src/admin/components/EditEventPage/index.tsx

-  const EditEventCustomForm = (EditEventForm as unknown) as React.ComponentClass<any>;
+  const EditEventCustomForm = EditEventForm as unknown as React.ComponentClass<any>;


This is fine for now, I opened #551 which I think will address some of the weird Typescript casting we have throughout the code.

That must've come from prettier, I don't recall manually editing that. as is probably left associative so the parenthesis don't matter.

steets250 · 2022-03-14T04:40:24Z

src/admin/containers/CreateEventForm.tsx

@@ -64,4 +63,4 @@ const FormikCreateEventForm = withFormik({
  },
 })(CreateEventForm as React.FC);

-export default connect(null, { postEvent, copyLink })(FormikCreateEventForm);
+export default connect(null, { postEvent })(FormikCreateEventForm);


Are you addressing all instances of connect( in this PR? You mention this in your second point, but I'm still seeing instances of it in use.

The only instances where I left connect was with Formik since there isn't a way to access the useDispatch or useSelector hooks with the withFormik HOC. In order to remove the connect usage there, we'd have to rewrite the Formik code with Formik hooks or different component APIs which is probably better suited for another PR.

steets250 · 2022-03-14T05:25:28Z

src/auth/components/EmailVerificationPage/index.tsx

 import './style.less';
+import { verifyEmail } from '../../utils';


Can we keep import './style.less'; as the last import?

For every file that I modified, I ran Typescript's built in organizeImports command from the language server tsserver. I'm not sure if we can configure it, but I like having an automated, opinionated formatter for imports.

steets250 · 2022-03-14T05:25:40Z

src/auth/components/ResendEmailVerificationPage/index.tsx

 import './style.less';
+import { sendEmailVerification } from '../../utils';


See above comment.

same as above

steets250 · 2022-03-14T05:32:01Z

src/auth/containers/requireAdminAuth.tsx

@@ -1,50 +1,32 @@
 import React, { useEffect } from 'react';


The changes in this, requireAuth, and requireStoreAccess will need to be re-applied to Paul's new access protection component. We'll want to merge from master to incorporate those changes.

steets250 · 2022-03-14T05:36:54Z

src/layout/components/NavBarHorizontal/index.tsx

-          <NavTileItem icon={DashboardIcon} text="Dashboard" />
+          <NavTileItem icon={DashboardIcon} />
        </NavLink>
        <NavLink activeClassName="selected" to="/leaderboard">
-          <NavTileItem icon={LBIcon} text="Leaderboard" />
+          <NavTileItem icon={LBIcon} />
        </NavLink>
        <NavLink exact activeClassName="selected" to="/profile">
-          <NavTileItem icon={ProfileIcon} text="Profile" />
+          <NavTileItem icon={ProfileIcon} />
        </NavLink>
        <NavLink activeClassName="selected" to="/discord">
-          <NavTileItem icon={DiscordIcon} text="Discord" />
+          <NavTileItem icon={DiscordIcon} />
        </NavLink>
        {isAdmin && (
          <NavLink activeClassName="selected" to="/admin">
-            <NavTileItem icon={AdminIcon} text="Admin" />
+            <NavTileItem icon={AdminIcon} />


How do these changes tie in to redux stuff?

The text prop was unused in the NavTileItem component so I removed it. I think it came up when I was resolving lint warnings.

steets250 · 2022-03-14T05:48:18Z

src/store/localStorage.ts

+    const cart = JSON.parse(serializedCart);
+    return cart as Cart;
+  } catch (err) {
+    return {};


If we console.log for store, shouldn't we do the same for load? And perhaps it should be a notify so that the user sees that where was an error and it's not a mystery why something didn't save/load.

Good catch, I was planning on removing the log and doing what loadCart does. But I like the idea of adding a notify so something like:

try { // ... } catch (err) { notify(err); return {}; } // ...

Does that sound good?

StormFireFox1

Left some nits for any things Steven didn't mention that I thought would be important but otherwise, before we merge, I feel like we should still discuss the items I mentioned in my first comment before we merge.

StormFireFox1 · 2022-03-20T04:01:19Z

src/admin/adminSlice.ts

@@ -0,0 +1,36 @@
+/* eslint-disable no-param-reassign */


Why is this required?

The no-param-reassign rule in our eslint config has the option {props: true} (from airbnb) which forbids mutating properties of parameters to reduce side effects. However, since redux-toolkit uses immer for createSlice, it's more idiomatic to mutate the state argument when defining reducers since it'll be equivalent to copying the parameter and is generally easier to read as well. I still think it's a good rule, so I've just disabled it in all slice files since createSlice will usually mutate the state parameter.

redux-toolkit has a discussion on this which tldr: airbnb is too restrictive, just use {props: false} which is the eslint default anyway.

Thoughts?

StormFireFox1 · 2022-03-20T04:02:17Z

src/admin/components/AddAttendanceForm/index.tsx

-import React, { useState, ChangeEventHandler, FocusEventHandler, FormEventHandler } from 'react';
-import { AutoComplete, Checkbox, Form, Input, Button, Tooltip, Tag, Icon, Select } from 'antd';
+import { AutoComplete, Button, Checkbox, Form, Icon, Input, Select, Tag, Tooltip } from 'antd';
+import React, { FormEventHandler, useState } from 'react';


Typically, I think in React the 'react' import should always be first in file (unless you're in Next.js, where you don't need it). I might wrong on this, and CRA might have removed that need. We should readjust that in files where it happens.

same as above. I don't believe the react import needs to come first, but it may be more idiomatic. Also, I think you're referring to the new JSX Transform where the React import is not needed and JSX completely handled at compile time. Unless we need to use React 16.3, we could upgrade to 16.4 or 17 to take advantage of it. Facebook even provides a codemod to automatically remove the unneeded imports. All we'd have to do is upgrade react and react-scripts I believe.

yesean · 2022-03-29T10:41:55Z

This is some great work, @yesean, well done!

I've looked over your PR at a first glance, and overall, it's good! I haven't reviewed the business logic you've changed yet, but I'll probably do that after finals week. I think there's a couple questions that came up while I looked over it we might want to discuss:

Currently, most of each feature's utils.ts is effectively the API calls, whereas conventionally utilities were simply local functions we used to simplify business logic. Currently, @steets250 was moving API calls under an API folder of sorts, maybe we wanna do something similar here.

I don't really see the distinction between API calls and business logic as API calls feel like another step in the logic. Moreover, the calls are generally localized to a section of the store and not shared between sections, so to me, it makes sense to keep them at a section-level rather than the top-level. Perhaps, utils isn't best place for API calls but I still think they should be localized rather than a top level API folder. Of course, if a backend API package is introduced, this may all be irrelevant.

Also, on a more fun note, like you said, I don't think we need containers anymore! It's kinda hard to truly move away from them, considering how prominent they are all around the code, so I suggest we rename all our containers to pages, which makes more sense as to what purpose they serve; representing each page from our portal.

pages sound good to me, glad to be moving away from containers 😀

What do y'all think of this?

StormFireFox1 · 2022-04-02T00:35:45Z

Just to recap all the discussion from above and meetings:

Add ESLint plugin for organizing imports
Handle merge conflicts for Paul's PR
Disable ESLint rule for no-param-reassign

yesean · 2022-04-26T04:50:39Z

Just to recap all the discussion from above and meetings:

Add ESLint plugin for organizing imports

Handle merge conflicts for Paul's PR

Disable ESLint rule for no-param-reassign

Fixed

StormFireFox1

Totally looks good to me! 🚀 👀

Redux Cleaning (#550) fails to run at production while containing too many file changes to individually bug fix and address. Thus, a decision was made to revert this commit and all commits after this commit was made.

steets250 and others added 24 commits January 11, 2022 00:42

Remove ThunkActionCreator

82b4beb

connect(null)

5bbff2a

Add redux-toolkit and slices

187e8ad

Merge branch 'master' into redux-cleaning

b743420

Independent History (#543)

07a204f

* Separate History from Redux * Update References to History * Remove connected-react-router * Update Location Usage Co-authored-by: Matei-Alexandru Gardus <[email protected]> Co-authored-by: Steven Steiner <[email protected]>

Remove ThunkActionCreator

c382499

Refactor store setup into redux/store.ts

47c55b0

Upgrade typescript, update eslint

8dd60ff

Add utils

63213ba

Finish storeSlice

db03213

Add authSlice

adc4fb4

Fix login issue

d34f3e3

Force usage of new typescript eslint parser (overrides old CRA depede…

efe7c48

…ncy)

Add adminSlice

3290754

Update authSlice

9a81b47

Add profileSlice

996d2a5

Add leaderboardSlice

6de7aed

Update authslice

e58d14c

Update adminSlice

f292920

Add eventSlice

5e24545

Update slices

3542cf8

Replace connect with hooks

5674c75

Remove logs

948cbac

Fix linting/compile errors

16b5d70

yesean added the PR: Needs Review This PR needs review label Mar 11, 2022

yesean self-assigned this Mar 11, 2022

yesean requested review from steets250 and StormFireFox1 March 11, 2022 04:25

steets250 reviewed Mar 14, 2022

View reviewed changes

StormFireFox1 suggested changes Mar 20, 2022

View reviewed changes

yesean added 4 commits April 25, 2022 17:54

reorganize imports

40345ec

disable eslint rule

6c4496a

merge master

0932c3a

Merge branch 'master' into redux-cleaning

c6b8a26

StormFireFox1 approved these changes May 3, 2022

View reviewed changes

Merge branch 'master' into redux-cleaning

d5d6581

yesean merged commit 2d4bdb8 into master May 3, 2022

yesean deleted the redux-cleaning branch May 3, 2022 03:34

This was referenced Oct 19, 2022

Reverted Redux Cleaning and subsequent commits #586

Closed

Reverted Redux Cleaning and subsequent commits #587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redux cleaning #550

Redux cleaning #550

yesean commented Mar 11, 2022 •

edited

Loading

StormFireFox1 commented Mar 13, 2022

steets250 commented Mar 13, 2022

steets250 Mar 14, 2022

yesean Mar 29, 2022

steets250 Mar 14, 2022

yesean Mar 29, 2022

steets250 Mar 14, 2022

yesean Mar 29, 2022

steets250 Mar 14, 2022

yesean Mar 29, 2022

steets250 Mar 14, 2022

steets250 Mar 14, 2022

yesean Mar 29, 2022

steets250 Mar 14, 2022 •

edited

Loading

yesean Mar 29, 2022

StormFireFox1 left a comment

StormFireFox1 Mar 20, 2022

yesean Mar 29, 2022

StormFireFox1 Mar 20, 2022

yesean Mar 29, 2022 •

edited

Loading

yesean commented Mar 29, 2022

StormFireFox1 commented Apr 2, 2022

yesean commented Apr 26, 2022

StormFireFox1 left a comment

		const EditEventCustomForm = (EditEventForm as unknown) as React.ComponentClass<any>;
		const EditEventCustomForm = EditEventForm as unknown as React.ComponentClass<any>;

		import './style.less';
		import { verifyEmail } from '../../utils';

		import './style.less';
		import { sendEmailVerification } from '../../utils';

Redux cleaning #550

Redux cleaning #550

Conversation

yesean commented Mar 11, 2022 • edited Loading

StormFireFox1 commented Mar 13, 2022

steets250 commented Mar 13, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steets250 Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StormFireFox1 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

yesean Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

yesean commented Mar 29, 2022

StormFireFox1 commented Apr 2, 2022

yesean commented Apr 26, 2022

StormFireFox1 left a comment

Choose a reason for hiding this comment

yesean commented Mar 11, 2022 •

edited

Loading

steets250 Mar 14, 2022 •

edited

Loading

yesean Mar 29, 2022 •

edited

Loading