Skip to content

Add Redux #18

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

Merged
merged 1 commit into from
Apr 22, 2023
Merged

Conversation

alexandersmanning
Copy link
Contributor

@alexandersmanning alexandersmanning commented Apr 21, 2023

This adds redux to the app, replace our own built in state module. I have manually tested a number of things and have ensured the current test work. With that said, I recommend you verify this too.

This is all following the quick start guide: https://redux-toolkit.js.org/tutorials/quick-start

Why redux:
Given that we are using immer and a single reducer, Redux provides us the same functionality, but with significantly better dev tools to watch state.

In the longterm, we should start using middleware to perform requests, which will help simplify the flow significantly, and allow us to better track where and when requests are being made. One of the cool things about Redux Tool Kit (which is what I am using) is that it provides something calls RTKQuery. This will allow us to handle loading states and such for requests a lot easier, and not have to create different actions to do so.

The app with Redux Dev Tools:

dev-tools

@alexandersmanning alexandersmanning marked this pull request as draft April 21, 2023 05:12
@egonSchiele
Copy link
Owner

Awesome!

@alexandersmanning alexandersmanning force-pushed the amanning/add-redux branch 2 times, most recently from cf33441 to a492d30 Compare April 22, 2023 17:44
@@ -0,0 +1,17 @@
const localStorageMock = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the initial state calls local storage, and the utils file needs a dispatcher, this sadly forces Jest to call local storage which does not exist. This mock is put in place to fix it.

@@ -36,7 +38,7 @@ async function newBook(dispatch) {
} else {
const book = res.payload;
console.log("new book", book);
dispatch({ type: "ADD_BOOK", payload: book });
dispatch(librarySlice.actions.addBook(book));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the quick start we can make this even easier:
https://redux-toolkit.js.org/tutorials/quick-start

by doing something like: export const { increment, decrement, incrementByAmount } = librarySlice.actions

The problem is we have A LOT of actions. I think this is something we can reevaluate and clean up later

import { localStorageOrDefault } from "../utils";

// @ts-ignore
const { createSlice } = toolkitRaw.default ?? toolkitRaw;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this piece is a bug with redux dealing with how ESM modules work in node. This has been fixed in v2.0.0, however that is still in alpha.

This is the work around for the time being:
reduxjs/redux-toolkit#1960

@@ -44,6 +44,8 @@ export type EditorState = {
_cachedSelectedText?: SelectedText;
_pushTextToEditor?: string;

_pushContentToEditor?: string;
Copy link
Contributor Author

@alexandersmanning alexandersmanning Apr 22, 2023

Choose a reason for hiding this comment

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

This is the only major change that you should find in the librarySlice file.

Right now you are saving the full quill object to the store, but you should never store non-serializable objects to a store. While this does not really affect us (you don't want to do this due to server side rendering and hydration), it is smart to follow the correct patterns.

I have modified the content to use this instead.

@alexandersmanning alexandersmanning marked this pull request as ready for review April 22, 2023 17:52
@egonSchiele egonSchiele merged commit 22077ba into egonSchiele:main Apr 22, 2023
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.

2 participants