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

Migrate to Redux Toolkit part 7 #2971

Merged
merged 27 commits into from
May 3, 2024
Merged

Migrate to Redux Toolkit part 7 #2971

merged 27 commits into from
May 3, 2024

Conversation

RichDom2185
Copy link
Member

Description

Based on @leeyi45's work done in #2874.

Intentionally split and only migrated some stuff to keep the diff readable.

I strongly recommend going through the changes commit by commit instead of going through the full diff of this PR. Please refer to the commit history for a list of changes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Everything should still work. Tests involving createStore are still failing at the moment, will fix in the next few days.

Checklist

  • I have tested this code
  • I have updated the documentation

RichDom2185 and others added 10 commits May 2, 2024 02:55
Originally done in f1cf593.

Amended to fix some bugs with types.

---------

Co-authored-by: Lee Yi <leeyi45@gmail.com>
Type errors will be fixed later.
Done to align with where the action creator is located.
Original implementation in bbf4f09.

Adapted to match current state of codebase.

---------

Co-authored-by: Lee Yi <leeyi45@gmail.com>
Also added a TODO for a testing util.
@chownces
Copy link
Contributor

chownces commented May 2, 2024

Turns out saferTakeEvery is not so safe 🤓. Added the fix.

@coveralls
Copy link

coveralls commented May 2, 2024

Pull Request Test Coverage Report for Build 8937016269

Details

  • 196 of 423 (46.34%) changed or added relevant lines in 13 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 32.731%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commons/achievement/AchievementOverview.tsx 0 2 0.0%
src/commons/achievement/AchievementView.tsx 0 3 0.0%
src/commons/redux/utils.ts 12 22 54.55%
src/features/stories/StoriesActions.ts 5 17 29.41%
src/commons/sagas/StoriesSaga.ts 1 18 5.56%
src/commons/application/actions/SessionActions.ts 52 70 74.29%
src/commons/mocks/BackendMocks.ts 12 34 35.29%
src/commons/sagas/BackendSaga.ts 107 250 42.8%
Files with Coverage Reduction New Missed Lines %
src/commons/sagas/BackendSaga.ts 5 43.14%
Totals Coverage Status
Change from base Build 8936341133: -0.6%
Covered Lines: 5146
Relevant Lines: 14691

💛 - Coveralls

@RichDom2185
Copy link
Member Author

Turns out saferTakeEvery is not so safe 🤓. Added the fix.

Thanks! My bad, I missed out the fact that saferTakeEvery takes in the action and not the action type (like takeEvery) 😅

Some are left unmigrated to preserve the readability of the diff.
Only updated sagas that require a simple fix of the imported action
types. Remaining sagas will be done in a later commit.
To minimise potentially unnecessary development effort while pending
removal decision outcome. Only commented handlers that are incompatible
with the type changes.
Use the action creator instead of creating the action object manually to
enforce abstraction.
Intentionally done in a very specific way, mixing new and old code
styles, in order to keep the diff legible for such a large migration.
Further refactoring will be done in the future.
@RichDom2185 RichDom2185 marked this pull request as ready for review May 3, 2024 09:11
src/commons/mocks/BackendMocks.ts Outdated Show resolved Hide resolved
src/commons/utils/TestUtils.ts Show resolved Hide resolved
Copy link
Member Author

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Tested locally and everything still works, will merge now and make the rest of the changes in subsequent parts.

@RichDom2185 RichDom2185 merged commit 637437f into master May 3, 2024
8 checks passed
@RichDom2185 RichDom2185 deleted the rtk-7 branch May 3, 2024 09:44
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