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

Install and configure Redux Toolkit #2187

Merged
merged 10 commits into from
Jan 2, 2024

Conversation

lindapaiste
Copy link
Collaborator

This is step 1 of issues #2042 and #2186

Implements the recommended first step in the Migrating to Modern Redux guide

Replace the existing manual Redux store setup with Redux Toolkit's configureStore

Changes:

  • Install package @reduxjs/toolkit.
  • Rewrite store setup using configureStore.
  • Update redux and @redux-devtools packages to their latest versions to resolve a warning:

Symbol.observable as defined by Redux and Redux DevTools do not match. This could cause your app to behave differently if the DevTools are not loaded. Consider polyfilling Symbol.observable before Redux is imported or avoid polyfilling Symbol.observable altogether.

  • Allow RTK to handle syncing the store with the browser DevTools extension, and only use the DockMonitor if the browser extension is not installed. Using both causes problems.
  • Change checks of deprecated window.devToolsExtension to current window.__REDUX_DEVTOOLS_EXTENSION__.
  • Remove reselect as a top-level dependency because it is bundled with RTK.
  • Change imports of createSelector from reselect to @reduxjs/toolkit.
  • Create an instance of listenerMiddleware and link it to the store. This is just a tool which can be used in future PRs and does not do anything right now.

Note: redux-thunk is also bundled with RTK, but it needs to stay as dependency because is used directly in some test files.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Questions:

  • I kept the hot module replacement code in the store setup but I don't know if it's actually needed. HMR has never worked for me so I cannot test it out. This answer from 2016 shows the exact setup that we have, but the answers from 2019 and 2020 say it's not needed.
  • I don't understand the use of window.__INITIAL_STATE__ in our index.jsx. It seems like it's used as a way to pass state from the backend to the frontend, but I don't see anywhere in the server part where we are setting this property.
  • Now that we have support for the Redux DevTools browser extension, which happens automatically with configureStore, I'm not sure if we really need the manual setup of the DockMonitor on top of that?

Here's what it looks like in Edge, where I have no browser Redux DevTools, and the DockMonitor is shown:
image

In Chrome, the DockMonitor won't show up on top of the web app anymore but the browser Redux DevTools work! Those tools show the Log but also a bunch of other stuff.
image
image
image
image

@raclim raclim added the Create Release Environment For use with ReleaseHub ephemeral environments label Apr 14, 2023
@lindapaiste lindapaiste mentioned this pull request May 7, 2023
4 tasks
…oolkit

# Conflicts:
#	client/modules/App/App.jsx
#	package-lock.json
@raclim raclim added the Area: Dependencies Pull requests that update a dependency file label Nov 3, 2023
@raclim raclim added this to the MINOR Release for 2.10.0 milestone Nov 7, 2023
Copy link
Contributor

@PiyushChandra17 PiyushChandra17 left a comment

Choose a reason for hiding this comment

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

Nicely Done 👍

@raclim
Copy link
Collaborator

raclim commented Dec 15, 2023

Hey! Just wanted to ask here since this PR should definitely get in before the end of the year 😅 if you're okay with having your PR edited by others (like in the case of merge conflicts), of if you'd prefer to handle them yourself! (I'm not sure if there's an etiquette for this?)

@lindapaiste
Copy link
Collaborator Author

Hey! Just wanted to ask here since this PR should definitely get in before the end of the year 😅 if you're okay with having your PR edited by others (like in the case of merge conflicts), of if you'd prefer to handle them yourself! (I'm not sure if there's an etiquette for this?)

Hey I'm fine with you doing it if you understand the conflicts. Like I definitely don't care if you regenerate a package-lock.json file. There may be cases where it makes sense for me to do it if the conflicts are more complicated.

I'm looking at this now though and hopefully I can resolve this one.

Sometimes I have problems when I go to merge in the develop branch because the merge commit won't go through if it fails any tests. My process in that case is:

  • Remove "pre-commit": "lint-staged" from the husky section of package.json file (but don't commit this).
  • Merge in upstream/develop.
  • Fix issues so that it passes all tests and commit those changes (usually as an amend to the merge commit).
  • Push.

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks for working through the conflicts! I'm going to quickly merge this in now then while it's good.

@raclim raclim merged commit 73d6234 into processing:develop Jan 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Dependencies Pull requests that update a dependency file Create Release Environment For use with ReleaseHub ephemeral environments
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants