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

Redux Toolkit Migration #1113

Merged
merged 141 commits into from
Mar 6, 2024
Merged

Redux Toolkit Migration #1113

merged 141 commits into from
Mar 6, 2024

Conversation

ChrisMart21
Copy link
Contributor

@ChrisMart21 ChrisMart21 commented Jan 15, 2024

Description

This pull request primarily focuses on updating the project to modern Redux patterns by migrating to the recommended 'Redux-Toolkit' while addressing several other minor issues along the way.

Fixes:

Partially addresses

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

  • This pull request introduces numerous changes across the front end that are yet to be thoroughly tested. While not entirely feature-complete, it represents a significant step towards migrating to RTK. A thorough evaluation is necessary to ensure these changes have not impaired the functionality of the app.

    • Close evaluation and iterations have been ongoing since initial posting of PR. It is reaching its final stages, and is believed to be stable. Further testing and evaluation from the community is always welcome, and encouraged.
  • This PR has a version bump to react-router-v6. This change introduces several newer hooks that pair well with the ongoing migration towards functional components. This did however introduce some backwards compatibility issues with the current implementation of the unsaved warning. This component has to be reworked for compatibility with react-router-v6, but also Redux RTK.

  • There was issues/difficulty migrating the entirety of 'Maps'. Maps currently is still using legacy redux and either needs to be reworked, or migrated properly by someone more knowledgeable on this section of the code base.

    • While maps utilizes some newer RTK elements, it is still heavily reliant on legacy redux patterns.
    • Further work is necessary on this section of the migration.
  • Redux-Toolkit introduces some new devmode checks which conflict with some of the legacy Redux code. For example, the 'serializableCheck' has to currently be suppressed due to the use of TimeInverval in Redux State. TimeInterval is a class which is non-serialiazable which needs to be addressed. Additionally 'immutableCheck' is also currently has conflicts with the maps implementation. For the time being, these checks are disabled to prevent the app from malfunctioning, however the goal is to address these issues, and have all dev mode checks enabled.

    • Progress has been made on this front and will likely be resolved in tandem with the Maps rework.

@huss
Copy link
Member

huss commented Mar 6, 2024

I want to express my deep thanks to @ChrisMart21 for initiating and completing the migration of OED to the Redux Toolkit and Query system. This not only gets OED up to the latest Redux usage but adds a number of new features such as history of graphics. This is a substantial step in OED keeping current with technology and reducing our technical debt. Many thanks.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

Thanks to @ChrisMart21 for another substantial contribution. All the comments have been addressed and testing finds the code works as expected. It is ready to merge.

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.

None yet

3 participants