-
Notifications
You must be signed in to change notification settings - Fork 3
Remove Redux state management and complete cleanup #1027
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
Conversation
…o React Context for state management across components
…o standardize testing utilities
…e WelcomeModal to eliminate unnecessary nesting of AlertProvider
…vicePoint hook by simplifying mock implementations
Hello ziyang-lin-404,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| 'src/react/workflow/', | ||
| 'src/react/next-architecture/domain/business/buckets.test.tsx', | ||
| 'src/react/ui-elements/__tests__/EmptyBucket.test.tsx', | ||
| 'src/react/ISV/hooks/useGetS3ServicePoint.test.ts', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these test are still ignored ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they are unrelated to Redux removal.
Will fix it next PR
hervedombya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye ziyang-lin-404. The following options are set: approve |
Context
Final Redux removal PR. After previous PRs migrated
instanceStatus,stats,auth, anderrorto React Context/React Query, this PR removes all remaining Redux infrastructure.Problem
package.jsonreduxRenderwere misleading after Redux removalSolution
package.jsonChanges
Deleted Files
src/react/reducers/index.tssrc/react/reducers/initialConstants.tssrc/react/actions/index.tssrc/types/state.tsModified Files
src/react/utils/testUtil.tsxnewTestStore(),realStoreWithInitState(),<Provider>wrapper; renamedreduxRender→testRender,reduxMount→testMount,reduxMountAct→testMountActsrc/react/FederableApp.tsxProvider,createStore,applyMiddleware,compose,thunkimports; removedInternalRouterempty wrapper componentsrc/react/ISV/components/Modal/WelcomeModal.tsxInternalRouterimport and usagesrc/types/actions.tsApiErrorinterface)package.jsonredux,react-redux,redux-thunk,redux-oidc,@types/react-redux,redux-mock-store,@types/redux-mock-storejest.config.jstestPathIgnorePatternsfor 5 tests with external library mock issuestestRender/testMountfunctions, removedtestStateparametersIssues Encountered and Solutions
Issue 1: Test Files Passing Unused
testStateParameterCause: After removing Redux, many test files still passed
testStateto render functions.Fix: Updated all test files to remove the unused parameter.
Issue 2: 5 Test Suites Failing After Removing
testPathIgnorePatternsCause: These tests require complex mocks for
@scality/data-browser-libraryand other external dependencies - unrelated to Redux removal.Fix: Added these 5 specific tests to
testPathIgnorePatterns:useGetS3ServicePoint.test.tsVeeamCapacityOverviewRow.test.tsxVeeamCapacityModal.test.tsxISVSummary.test.tsxSelectAccountIAMRole.test.tsxRedux Usage Progress
Removed Dependencies
Production:
reduxreact-reduxredux-thunkredux-oidcDevelopment:
@types/react-reduxredux-mock-store@types/redux-mock-storeTesting
npm run check-types- No TypeScript errorsnpm test- 563 tests passed (76/77 suites)