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

DATAP-1208 Convert reducers to RTK slices #482

Merged
merged 125 commits into from
Oct 23, 2024

Conversation

wwhorton
Copy link
Contributor

@wwhorton wwhorton commented Nov 9, 2023

Redux Toolkit divides redux state into "slices", which make managing state easier and more efficient. Explorer was converted to use slices, but CCDB hadn't yet. This is a PR converting all the reducers to slices, updating tests to accommodate the changes, and refactoring code where necessary, mainly in dispatches. Most of the content of the actions directory was removed since it's handled in the extraReducers piece of the slices, but where it seemed to make more sense to leave them as is, I did.

Removals

  • boilerplate action creators
  • boilerplate reducer setup
  • snapshot tests
  • redux-mock-store

Changes

  • numerous, but, broadly speaking, all of the reducers are now slices and have been converted in place
  • the store was updated to use the newer RTK format
  • added testing-library tests instead of snapshot tests
  • added test setup using real reducers and actions
  • added logic from Complaint Explorer
  • split off logic from Query reducer into Filters, Map, and Trends reducers
  • added middleware to bring together parameters to query the api
  • added middleware logic to only query the aggregations endpoint only when required instead of currently querying both results and aggregations no matter what
  • added React strict mode to catch more errors during development
  • Fix company typeahead

Testing

  • yarn test: All tests should pass
  • Since redux touches all parts of the app, it would probably be a good idea to go through manually and make sure that everything looks as it should and works as expected.

Notes

  • The names of the tests should probably be updated to reflect the new reducer action names. It didn't occur to me until pretty late in the process, but I don't want to hold up the PR to make those updates when they could be a separate one.
  • Something unexpected I learned was that when state updates in actions relied on a return value from enforceValues() they didn't actually update with the new value if it was within a returned object, e.g. { ...state, newValue: enforceValues(x,'y') }. It had to be written in the "state.value = enforceValues(x, 'y')" form. I think this might have something to do with Immer, but it's probably a good idea for all the state updates to take that form. Again, probably ought to be another PR to update that in the places it wasn't necessary for this PR.

Todos

  • Go through and prune away tests that are no longer required, and add tests to meet coverage requirements. Because the conversion alone results in an enormous PR I thought it would be a good idea to do this part as a separate PR.
  • Optimize the slices and the action directory. Again, to avoid complicating what was already a huge PR, I didn't do this, preferring to set it aside as a separate PR.

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

@flacoman91 flacoman91 force-pushed the WW-DATAP-1208-reducers-use-slices branch 5 times, most recently from 615106a to 2d0a445 Compare October 15, 2024 23:59
@higs4281 higs4281 marked this pull request as ready for review October 16, 2024 13:56
@flacoman91 flacoman91 force-pushed the WW-DATAP-1208-reducers-use-slices branch from 21c95bd to 7192be5 Compare October 16, 2024 19:27
@flacoman91 flacoman91 force-pushed the WW-DATAP-1208-reducers-use-slices branch 2 times, most recently from 59e2d92 to 58b7acb Compare October 18, 2024 13:42
@cdmh219
Copy link
Collaborator

cdmh219 commented Oct 21, 2024

Nothing needed from this comment, just a suggestion. Now that we're on RTK, do you think RTK Query could be a good option for our API calls? Seems like it would eliminate a lot of the custom structure we've put in place to create and make API calls. May be something to look into down the line.

@flacoman91
Copy link
Contributor

Nothing needed from this comment, just a suggestion. Now that we're on RTK, do you think RTK Query could be a good option for our API calls? Seems like it would eliminate a lot of the custom structure we've put in place to create and make API calls. May be something to look into down the line.

Yes, great suggestion. I'm all for anything that reduces custom code and not reinventing the wheel!

@cdmh219
Copy link
Collaborator

cdmh219 commented Oct 21, 2024

Also doesn't look like that RTK's createAsyncThunk() is being used for API calls. RTK Query is a separate library and long-term solution that may or may not be a good fit, but createAsyncThunk() could be used immediately, as it is an out-of-the-box solution for properly integrating Redux with API calls. That will also eliminate much of our custom logic. More about this can be found here

src/App.spec.js Outdated Show resolved Hide resolved
src/utils/index.js Outdated Show resolved Hide resolved
src/actions/routes.js Outdated Show resolved Hide resolved
src/components/ComplaintDetail/ComplaintDetail.spec.js Outdated Show resolved Hide resolved
src/reducers/map/selectors.js Outdated Show resolved Hide resolved
src/reducers/trends/trendsSlice.js Outdated Show resolved Hide resolved
src/reducers/trends/trendsSlice.js Outdated Show resolved Hide resolved
src/reducers/trends/trendsSlice.js Outdated Show resolved Hide resolved
src/reducers/trends/trendsSlice.js Show resolved Hide resolved
complaints.spec fix

fixing unit test coverage

updating unit tests

update unit test, remove dead code

update selectors, fix trends test

fixing a test

remove unused isfromexternal param

comment out unused selectors

remove unused code

remove unused code

remove unused code

remove unused code

adding unit test for search component

fix comment

updating test

fix cypress test in doc detail view

fixing test

fixing some unit tests

revert cypress fixes

fixing tests

adding pager reset when date change

fixing test

fixing unit tests

fixing unit tests

fixing unit tests

add dist

squash fixtures, remove mutation observer, doesnt seem like it is needed

squash

update gitignore

move test files

fixing test setup, update coverage package.json

fixing tests

refactoring payload reducer

fix typeahead

bug fixes, fixing pagination, hide when no results

linting

update dist

update date model, correct time zone

accessiblity fixes for date range, expand/collapse filter

linting

complaint and apostrophe fix

remove breaks

fixing tests

updating build

update feedback

cleanup

update stuff

cleanup
…-1208-reducers-use-slices

# Conflicts:
#	dist/ccdb5.css
#	dist/ccdb5.css.map
#	dist/ccdb5.js
#	dist/ccdb5.js.map
#	src/components/Filters/DateRanges.js
#	src/components/List/ComplaintCard/ComplaintCard.js
#	src/components/List/ListPanel/ListPanel.js
#	src/components/Map/MapToolbar.js
#	src/components/Map/TileChartMap/TileChartMap.js
#	src/components/RefineBar/ChartToggles.js
#	src/components/Search/Pill.js
#	src/components/Search/SearchBar.js
#	src/components/Trends/LensTabs.js
#	src/components/Trends/TrendDepthToggle.js
@flacoman91 flacoman91 force-pushed the WW-DATAP-1208-reducers-use-slices branch from b6f136d to ec3a36e Compare October 22, 2024 22:18
@cdmh219 cdmh219 self-requested a review October 23, 2024 19:47
@flacoman91 flacoman91 added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 800f2f1 Oct 23, 2024
2 checks passed
@flacoman91 flacoman91 deleted the WW-DATAP-1208-reducers-use-slices branch October 23, 2024 20:00
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.

3 participants