Conversation
There was a problem hiding this comment.
Pull request overview
This pull request installs Redux Toolkit (RTK) 2.11.2 and upgrades core Redux dependencies to their latest major versions (Redux 5.0.1, redux-thunk 3.1.0, reselect 5.1.0). The primary change is updating all test files to use the named export for redux-thunk middleware, which is a breaking change in redux-thunk 3.x. The PR sets up the foundation for RTK Query by creating a base API configuration with authentication and tracing headers, though no actual API endpoints are defined yet (those will be added in PR #1865).
Changes:
- Installed @reduxjs/toolkit 2.11.2 and upgraded Redux from 4.2.1 to 5.0.1, redux-thunk from 2.4.2 to 3.1.0, and reselect from 4.1.8 to 5.1.0
- Updated all test files (60+ files) to import redux-thunk middleware as a named export
{ thunk }instead of default export - Created RTK Query base API configuration with authentication headers, retry logic, and action dispatching for tracking
- Integrated RTK Query reducer and middleware into both dev and production store configurations
- Updated tidepool-platform-client to pre-release version 0.65.0-web-4390-install-rtk.1 (likely includes getSessionTrace method)
Reviewed changes
Copilot reviewed 62 out of 63 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added @reduxjs/toolkit dependency and upgraded redux, redux-thunk, reselect, and tidepool-platform-client versions |
| yarn.lock | Updated lockfile with new dependency versions and their transitive dependencies |
| app/redux/api/baseApi.js | Created new RTK Query base API with authentication, tracing, retry logic, and custom action dispatches |
| app/redux/constants/actionTypes.js | Added RTK_QUERY_REQUEST_SUCCESS and RTK_QUERY_REQUEST_ERROR action type constants |
| app/redux/store/configureStore.prod.js | Integrated RTK Query reducer and middleware into production store configuration |
| app/redux/store/configureStore.dev.js | Integrated RTK Query reducer and middleware into development store configuration with hot module replacement support |
| test/utils/mountWithProviders.js | Updated redux-thunk import to use named export |
| test/unit/**/*.test.js (30+ files) | Updated all test files to import redux-thunk as named export { thunk } |
| tests/unit/**/*.test.js (28+ files) | Updated all Jest test files to import redux-thunk as named export { thunk } |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const getSessionToken = () => { | ||
| return keycloak?.token || ''; | ||
| }; |
There was a problem hiding this comment.
The abstraction of getSessionToken() and getSessionTrace() may be a pointless exercise, but I wanted to create some level of separation such that the query call isn't calling on keycloak or tidepoolApi directly. A functional-style dependency-injection, if you will
| const reducer = combineReducers({ | ||
| blip: reducers, | ||
| router: connectRouter(history), | ||
| [RTKQueryApi.reducerPath]: RTKQueryApi.reducer, |
There was a problem hiding this comment.
This creates a new top-level redux slice called api, so it's neatly separated from all existing blip data which is nice.
|
Hi @ginnyyadav , I am tagging you in case you want to take a look (at your leisure). The code changes for this are relatively small (<80 lines), the other ~60 file changes are just changing an import style due to breaking changes on a library upgrade for a dependency. The changes effectively are, across 3 files:
Hoping that this can help inform QA effort or help you raise any questions/concerns that should be pre-emptively addressed. As it stands, once we install RTK with this PR, nothing should change; just need to look out for any regressions. There are potential breaking changes with the dependency updates but I haven't found any issues aside from the import style. |
[WEB-4390] - Install RTK - Add MSW for testing
| @@ -1,5 +1,5 @@ | |||
| module.exports = { | |||
| testEnvironment: 'jsdom', | |||
| testEnvironment: 'jest-fixed-jsdom', | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
We could potentially extend it ourselves instead (it's not a lot of code) if we don't want to rely yet another package.
There was a problem hiding this comment.
I think it's fine to pull this in. Doing it ourselves gives us the same result with likely more maintenance.
| languageName: node | ||
| linkType: hard | ||
|
|
||
| "@inquirer/ansi@npm:^1.0.2": |
There was a problem hiding this comment.
Essentially all of this stuff is from msw
| }; | ||
|
|
||
| const apiHost = config.API_HOST; | ||
| const apiVersion = 'v1'; |
There was a problem hiding this comment.
I notice that we hardcode v1 in platform-client everywhere instead of putting it in config or something. I was thinking about putting it in config or at some higher level, but it seems like something we are intentionally not doing that
There was a problem hiding this comment.
It's good here for now. Who knows if we ever will see a v2, but it's good to split the version out at any rate.
| }, | ||
| }); | ||
|
|
||
| export const RTKQueryApi = createApi({ |
There was a problem hiding this comment.
bad name?
A conventional name might be baseApi or emptySplitApi but now we have two api objects in blip.
There was a problem hiding this comment.
I'd lean towards baseApi to match the filename. Any good code editor intellisense should allow us to easily find it's definition and what it does.
You're fine to leave the name as-is too. I'm fine either way.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 67 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| global.__I18N_ENABLED__ = 'false'; | ||
| global.__DEV_TOOLS__ = false; | ||
|
|
||
| window.config = { |
There was a problem hiding this comment.
window.config is assigned a new object here, which will overwrite any existing config keys that a test (or another setup file) may have set. To avoid clobbering other settings, consider merging into the existing object (e.g., initialize window.config if missing and then set API_HOST).
| window.config = { | |
| window.config = { | |
| ...(window.config || {}), |
There was a problem hiding this comment.
I think it's fine as-is for now
There was a problem hiding this comment.
Yeah, the whole window.config init makes me scratch my head every now and again, but agree it's fine.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
clintonium-119
left a comment
There was a problem hiding this comment.
Everything looks great to me. Nice clean work on this :)
LGTM 🚢
| const reducer = combineReducers({ | ||
| blip: reducers, | ||
| router: connectRouter(history), | ||
| [RTKQueryApi.reducerPath]: RTKQueryApi.reducer, |
| @@ -1,5 +1,5 @@ | |||
| module.exports = { | |||
| testEnvironment: 'jsdom', | |||
| testEnvironment: 'jest-fixed-jsdom', | |||
There was a problem hiding this comment.
I think it's fine to pull this in. Doing it ourselves gives us the same result with likely more maintenance.
| global.__I18N_ENABLED__ = 'false'; | ||
| global.__DEV_TOOLS__ = false; | ||
|
|
||
| window.config = { |
There was a problem hiding this comment.
Yeah, the whole window.config init makes me scratch my head every now and again, but agree it's fine.
| }, | ||
| }); | ||
|
|
||
| export const RTKQueryApi = createApi({ |
There was a problem hiding this comment.
I'd lean towards baseApi to match the filename. Any good code editor intellisense should allow us to easily find it's definition and what it does.
You're fine to leave the name as-is too. I'm fine either way.
|
|
||
| const getSessionToken = () => { | ||
| return keycloak?.token || ''; | ||
| }; |
| }; | ||
|
|
||
| const apiHost = config.API_HOST; | ||
| const apiVersion = 'v1'; |
There was a problem hiding this comment.
It's good here for now. Who knows if we ever will see a v2, but it's good to split the version out at any rate.
WEB-4390
platform-client: tidepool-org/platform-client#215
I know it's +600 lines but It's not that bad, just renaming a bunch of test cases to use the 'thunk' named export (breaking change)
You can't really test this manually, so here is a branch you can checkout if you want to see it in action:
#1865