Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize redux #214

Merged
merged 29 commits into from
Dec 19, 2023
Merged

Modernize redux #214

merged 29 commits into from
Dec 19, 2023

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Oct 16, 2023

Resolves #213.

This PR aims at laying the groundwork for modernizing redux. Thankfully, the modernization can happen incrementally, so this PR does not need to and likely will not update everything at once. It will however try to touch all the relevant parts so it can serve as a guideline for future PRs that seek to continue with the modernization.

As such, discussions on code styles and best practices are very welcome.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The first step to modernizing redux. Replacing `createStore` with
`configureStore` from the toolkit.
@Arnei Arnei added the type:enhancement New feature or request label Oct 16, 2023
@Arnei Arnei changed the title Modernize redux store Modernize redux Oct 16, 2023
Arnei added 4 commits October 16, 2023 16:30
Does not really do anything yet, but will be useful later.
Add some basic typescript to aclSlice.ts. Avoids the difficult types,
this is more about groundwork.
Instead of roundabout `connect`, `mapStateToProps` and
`mapDispatchToProps`, we now use the `useSelector` and
`useDispatch` hooks from redux toolkit. Rewriting them as
`useAppSelector` and `useAppDispatch` will hopefully help
with implicit typing when we get to modernizing thunks.
@Arnei Arnei marked this pull request as ready for review October 19, 2023 15:35
@Arnei
Copy link
Member Author

Arnei commented Oct 19, 2023

Marking this as ready for review. There's still a large part this PR is not touching on, namely the whole request and response stuff, but migrating that to RTK query is proving a bit difficult, so not sure when I'll get around to that.

Moves fetching code from aclThunks.ts to aclSlice.ts. Also uses
createAsyncThunk, which tracks the state of the fetch request for us.

I'd rather have migrated to RTK Query instead of using createAsyncThunk,
but that is nearly impossible to do incrementally, making it a rather
large ask. Therefore, let's go with createAsyncThunk for now where
we can, which also helps streamlining the code and will hopefully
make a future migration to RTK Query easier. (Also RTK Query is still
kinda new).
Arnei added a commit to Arnei/opencast-admin-interface that referenced this pull request Nov 2, 2023
In the old admin ui, the status of workflow operations in the event
details was updated in real-time, allowing you to "watch" your
workflow running. This updates our admin ui to do the same by
re-fetching workflow operations every 5 seconds.

Unfortunately, this is causing rerenders even if the response
from the Opencast backend has not changed between calls.
We could probably memoize this somehow, but I can't find a
straightforward way, at least not one that is more straightfoward
than switching to redux toolkit (like in opencast#214).
@Arnei Arnei added type:code-enhancement Internal improvements to the codebase and removed type:enhancement New feature or request labels Nov 3, 2023
Copy link
Contributor

@owi92 owi92 left a comment

Choose a reason for hiding this comment

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

Mostly indentation/semicolon stuff, which should be easier to handle once "proper" linting gets introduced. So technically doesn't need to be adjusted in this PR.

I should mention however that I couldn't test this properly because of the error that I described to you in our meeting. I'm not sure what's happening there but since you can't reproduce, this should be fine to merge.

app/src/components/shared/MainNav.tsx Outdated Show resolved Hide resolved
app/src/components/shared/MainNav.tsx Outdated Show resolved Hide resolved
app/src/components/users/Acls.tsx Outdated Show resolved Hide resolved
app/src/components/users/Groups.tsx Outdated Show resolved Hide resolved
app/src/components/users/Users.tsx Outdated Show resolved Hide resolved
app/src/slices/aclSlice.ts Outdated Show resolved Hide resolved
app/src/store.ts Outdated Show resolved Hide resolved
app/src/store.ts Outdated Show resolved Hide resolved
app/src/thunks/tableThunks.ts Outdated Show resolved Hide resolved
app/src/thunks/tableThunks.ts Outdated Show resolved Hide resolved
app/src/slices/aclSlice.ts Outdated Show resolved Hide resolved
Arnei and others added 14 commits November 20, 2023 09:10
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Arnei and others added 9 commits November 20, 2023 09:33
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
Co-authored-by: Ole Wieners <94838646+owi92@users.noreply.github.com>
@Arnei
Copy link
Member Author

Arnei commented Nov 20, 2023

Thanks for the extensive linting. Any opinion on using redux toolkit and the way I went about it?

Also we should look into the error you got in more detail.

@owi92
Copy link
Contributor

owi92 commented Nov 20, 2023

Any opinion on using redux toolkit and the way I went about it?

Uhm not really since I haven't done anything with redux before. Which makes this a little hard to evaluate for me. Being a human linter is really all I can do for you here, for now. Maybe you'll want or need another reviewer for this after all.

Also we should look into the error you got in more detail.

Should I open an issue with a detailed report? I'm still wondering if this might be caused by something on my end, since you couldn't reproduce it.

@owi92
Copy link
Contributor

owi92 commented Nov 23, 2023

Any opinion on using redux toolkit and the way I went about it?

Sooo, after your introduction to redux and reading parts of the manual (which, let's be honest, I totally should have done in the first place instead of merely doing the more or less useless linting), I can say with a little more confidence that I think these changes are a nice first step in introducing the RTK. Anything done here makes sense to me, including sticking with createAsyncThunk for now. And I do also think migrating to RTK Query will be another nice improvement (for a future PR).

If you're comfortable with only having my reviews and approval for this, I'd say this can be merged. Though I would recommend that #212 gets merged prior to this (or at least very soon) to ease future development (and potential testing/reviews).

@Arnei
Copy link
Member Author

Arnei commented Dec 19, 2023

Going ahead and merging this, so work on this can continue. Had expected this to be more controversial, but maybe it just isn't.

@Arnei Arnei merged commit 33ca2df into opencast:admin-ui-picard Dec 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:code-enhancement Internal improvements to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modernize React Redux
2 participants