From a17a759d5e48afd44d5af45f2a28d7de33d87822 Mon Sep 17 00:00:00 2001 From: lucasgarfield Date: Wed, 31 Aug 2022 16:57:54 +0200 Subject: [PATCH] Redux: Modernize Redux store This commit modernizes the Redux store to bring it in line with the current recommended best practices/patterns. It is possible because of a recent previous commit that added Redux Toolkit as a dependency. For detailed information on what modern Redux entails, see the Redux docs: https://redux.js.org/introduction/getting-started#learn-modern-redux-livestream Practically speaking, this means a huge reduction in boilerplate code. Maintaining and extending the code will be much easier. All Redux logic is now encapsulated by 'slices'. Reducers are defined in the slices, and action creators and action types are generated automatically. Redux Toolkit includes Immer, which greatly simplifies writing reducer logic much simpler - state updates in Redux must be immutable, but working with Javascript objects in an immutable fashion is clumsy, requiring gratuitious use of the spread ... operator. With Immer, the state can be updated as if mutable updates were allowed. Reducer logic has been changed to take advantage of this in this commit. This commit also removes a large amount of unused code. Fingers crossed that nothing breaks. The tests do pass, afterall... One other minor change... composesGet was renamed to fetchComposes and composeGetStatus was renamed to fetchComposeStatus. This is in line with the Redux documentation examples. --- .../CreateImageWizard/CreateImageWizard.js | 10 +- src/Components/ImagesTable/ImagesTable.js | 10 +- src/store/actions/actions.js | 165 ++---------------- src/store/actions/index.js | 1 - src/store/composesSlice.js | 37 ++++ src/store/index.js | 4 +- src/store/reducers/composes.js | 102 ----------- src/store/types.js | 29 --- .../LandingPage/LandingPage.test.js | 4 +- src/test/redux/actions.test.js | 34 ---- src/test/redux/reducers.test.js | 54 +----- 11 files changed, 76 insertions(+), 374 deletions(-) delete mode 100644 src/store/actions/index.js create mode 100644 src/store/composesSlice.js delete mode 100644 src/store/reducers/composes.js delete mode 100644 src/store/types.js delete mode 100644 src/test/redux/actions.test.js diff --git a/src/Components/CreateImageWizard/CreateImageWizard.js b/src/Components/CreateImageWizard/CreateImageWizard.js index 4902d373..aa52797e 100644 --- a/src/Components/CreateImageWizard/CreateImageWizard.js +++ b/src/Components/CreateImageWizard/CreateImageWizard.js @@ -19,12 +19,12 @@ import { fileSystemConfigurationValidator, targetEnvironmentValidator, } from './validators'; +import { composeAdded } from '../../store/composesSlice'; import DocumentationButton from '../sharedComponents/DocumentationButton'; import './CreateImageWizard.scss'; import api from '../../api'; import { UNIT_GIB, UNIT_KIB, UNIT_MIB } from '../../constants'; import isRhel from '../../Utilities/isRhel'; -import { composeAdded } from '../../store/actions/actions'; const handleKeyDown = (e, handleClose) => { if (e.key === 'Escape') { @@ -427,14 +427,14 @@ const CreateImageWizard = () => { requests.map((request) => api.composeImage(request).then((response) => { dispatch( - composeAdded( - { + composeAdded({ + compose: { ...response, request, image_status: { status: 'pending' }, }, - true - ) + insert: true, + }) ); }) ) diff --git a/src/Components/ImagesTable/ImagesTable.js b/src/Components/ImagesTable/ImagesTable.js index 2361ba71..08c5851a 100644 --- a/src/Components/ImagesTable/ImagesTable.js +++ b/src/Components/ImagesTable/ImagesTable.js @@ -33,7 +33,7 @@ import Target from './Target'; import ImageLink from './ImageLink'; import ErrorDetails from './ImageBuildErrorDetails'; import DocumentationButton from '../sharedComponents/DocumentationButton'; -import { composeGetStatus, composesGet } from '../../store/actions/actions'; +import { fetchComposes, fetchComposeStatus } from '../../store/actions/actions'; const ImagesTable = () => { const [page, setPage] = useState(1); @@ -67,13 +67,13 @@ const ImagesTable = () => { return; } - dispatch(composeGetStatus(id)); + dispatch(fetchComposeStatus(id)); }); }; /* Get all composes once on mount */ useEffect(() => { - dispatch(composesGet(perPage, 0)); + dispatch(fetchComposes(perPage, 0)); }, []); /* Reset the polling each time the composes in the store are updated */ @@ -89,7 +89,7 @@ const ImagesTable = () => { if (composes.count > composes.allIds.length) { const pageIndex = page - 1; const offset = pageIndex * perPage; - dispatch(composesGet(perPage, offset)); + dispatch(fetchComposes(perPage, offset)); } setPage(page); @@ -102,7 +102,7 @@ const ImagesTable = () => { composes.count > composes.allIds.length && perPage > composes.allIds.length ) { - dispatch(composesGet(perPage, 0)); + dispatch(fetchComposes(perPage, 0)); } // page should be reset to the first page when the page size is changed. diff --git a/src/store/actions/actions.js b/src/store/actions/actions.js index 37d1ab10..91706047 100644 --- a/src/store/actions/actions.js +++ b/src/store/actions/actions.js @@ -1,158 +1,27 @@ +import { + composeAdded, + composesUpdatedCount, + composeUpdatedStatus, +} from '../composesSlice'; import api from '../../api'; -import types from '../types'; -function composeUpdated(compose) { - return { - type: types.COMPOSE_UPDATED, - payload: { compose }, - }; -} - -export const composeFailed = (error) => ({ - type: types.COMPOSE_FAILED, - payload: { error }, -}); - -export const composeAdded = (compose, insert) => ({ - type: types.COMPOSE_ADDED, - payload: { compose, insert }, -}); - -export const composeStart = (composeRequest) => async (dispatch) => { - // response will be of the format {id: ''} - const request = api.composeImage(composeRequest); - return request - .then((response) => { - // add the compose id to the compose object to provide access to the id if iterating through - // composes and add an image status of 'pending' alongside the compose request. - const compose = Object.assign( - {}, - response, - { request: composeRequest }, - { image_status: { status: 'pending' } } - ); - dispatch(composeAdded(compose, true)); - }) - .catch((err) => { - if (err.response.status === 500) { - dispatch(composeFailed('Error: Something went wrong serverside')); - } else { - dispatch(composeFailed('Error: Something went wrong with the compose')); - } - }); -}; - -export const composeUpdatedStatus = (id, status) => ({ - type: types.COMPOSE_UPDATED_STATUS, - payload: { id, status }, -}); - -export const composeGetStatus = (id) => async (dispatch) => { +export const fetchComposeStatus = (id) => async (dispatch) => { const request = await api.getComposeStatus(id); - dispatch(composeUpdatedStatus(id, request.image_status)); + dispatch( + composeUpdatedStatus({ + id, + status: request.image_status, + }) + ); }; -export const composesUpdatedCount = (count) => ({ - type: types.COMPOSES_UPDATED_COUNT, - payload: { count }, -}); - -export const composesGet = (limit, offset) => async (dispatch) => { +export const fetchComposes = (limit, offset) => async (dispatch) => { const request = await api.getComposes(limit, offset); request.data.map((compose) => { - dispatch(composeAdded(compose, false)); - dispatch(composeGetStatus(compose.id)); + dispatch(composeAdded({ compose, insert: false })); + dispatch(fetchComposeStatus(compose.id)); }); - dispatch(composesUpdatedCount(request.meta.count)); + dispatch(composesUpdatedCount({ count: request.meta.count })); }; -function setRelease({ arch, distro }) { - return { - type: types.SET_RELEASE, - payload: { - arch, - distro, - }, - }; -} - -function setUploadDestinations({ aws, azure, google }) { - return { - type: types.SET_UPLOAD_DESTINATIONS, - payload: { - aws, - azure, - google, - }, - }; -} - -function setUploadAWS({ shareWithAccounts }) { - return { - type: types.SET_UPLOAD_AWS, - payload: { - shareWithAccounts, - }, - }; -} - -function setUploadAzure({ tenantId, subscriptionId, resourceGroup }) { - return { - type: types.SET_UPLOAD_AZURE, - payload: { - tenantId, - subscriptionId, - resourceGroup, - }, - }; -} - -function setUploadGoogle({ accountType, shareWithAccounts }) { - return { - type: types.SET_UPLOAD_GOOGLE, - payload: { - accountType, - shareWithAccounts, - }, - }; -} - -function setSelectedPackages(selectedPackages) { - return { - type: types.SET_SELECTED_PACKAGES, - payload: selectedPackages, - }; -} - -function setSubscription({ activationKey, insights, organization }) { - return { - type: types.SET_SUBSCRIPTION, - payload: { - activationKey, - insights, - organization, - }, - }; -} - -function setSubscribeNow(subscribeNow) { - return { - type: types.SET_SUBSCRIBE_NOW, - payload: subscribeNow, - }; -} - -export default { - composesGet, - composeStart, - composeUpdated, - composeGetStatus, - setRelease, - setUploadDestinations, - setUploadAWS, - setUploadAzure, - setUploadGoogle, - setSelectedPackages, - setSubscription, - setSubscribeNow, -}; +export default { fetchComposes, fetchComposeStatus }; diff --git a/src/store/actions/index.js b/src/store/actions/index.js deleted file mode 100644 index 792ac234..00000000 --- a/src/store/actions/index.js +++ /dev/null @@ -1 +0,0 @@ -export { default as actions } from './actions'; diff --git a/src/store/composesSlice.js b/src/store/composesSlice.js new file mode 100644 index 00000000..b7a7906e --- /dev/null +++ b/src/store/composesSlice.js @@ -0,0 +1,37 @@ +import { createSlice } from '@reduxjs/toolkit'; + +const initialState = { + count: 0, + allIds: [], + byId: {}, + error: null, +}; + +const composesSlice = createSlice({ + name: 'composes', + initialState, + reducers: { + composeAdded: (state, action) => { + // only add to array if compose does not exist + if (!state.allIds.includes(action.payload.compose.id)) { + if (action.payload.insert) { + state.allIds.unshift(action.payload.compose.id); + } else { + state.allIds.push(action.payload.compose.id); + } + } + state.byId[action.payload.compose.id] = action.payload.compose; + state.error = null; + }, + composesUpdatedCount: (state, action) => { + state.count = action.payload.count; + }, + composeUpdatedStatus: (state, action) => { + state.byId[action.payload.id].image_status = action.payload.status; + }, + }, +}); + +export const { composeAdded, composesUpdatedCount, composeUpdatedStatus } = + composesSlice.actions; +export default composesSlice.reducer; diff --git a/src/store/index.js b/src/store/index.js index d467b636..c122fc57 100644 --- a/src/store/index.js +++ b/src/store/index.js @@ -1,10 +1,10 @@ import { configureStore } from '@reduxjs/toolkit'; import promiseMiddleware from 'redux-promise-middleware'; import { notificationsReducer } from '@redhat-cloud-services/frontend-components-notifications/redux'; -import composes from './reducers/composes'; +import composesSlice from './composesSlice'; export const reducer = { - composes: composes, + composes: composesSlice, notifications: notificationsReducer, }; diff --git a/src/store/reducers/composes.js b/src/store/reducers/composes.js deleted file mode 100644 index 121f3a5e..00000000 --- a/src/store/reducers/composes.js +++ /dev/null @@ -1,102 +0,0 @@ -import types from '../types'; - -// Example of action.compose -// { -// "77e4c693-0497-4b85-936d-b2a3ad69571b": { -// created_at: "2021-04-21 11:20:46.927594 +0000 UTC", -// id: "77e4c693-0497-4b85-936d-b2a3ad69571b", -// request: { -// distribution: "rhel-8", -// image_requests: [ -// { -// architecture: "x86_64", -// image_type: "ami", -// upload_request: { -// type: "aws", -// options: {} -// } -// } -// ] -// }, -// image_status: { -// status: "uploading", -// }, -// } -// }; - -const initialComposesState = { - count: 0, - allIds: [], - byId: {}, - error: null, -}; - -// only add to array if compose does not exist -const updateAllIds = (allIds, id, insert) => { - if (allIds.includes(id)) { - return allIds; - } - - if (insert) { - return [id].concat(allIds); - } - - return allIds.concat(id); -}; - -export function composes(state = initialComposesState, action) { - switch (action.type) { - case types.COMPOSE_ADDED: - return { - ...state, - allIds: updateAllIds( - state.allIds, - action.payload.compose.id, - action.payload.insert - ), - byId: { - ...state.byId, - [action.payload.compose.id]: action.payload.compose, - }, - error: null, - }; - case types.COMPOSE_FAILED: - return { - ...state, - error: action.payload.error, - }; - case types.COMPOSE_PENDING: - return { - ...state, - error: null, - }; - case types.COMPOSE_UPDATED: - return { - ...state, - byId: { - ...state.byId, - [action.payload.compose.id]: action.payload.compose, - }, - }; - case types.COMPOSES_UPDATED_COUNT: - return { - ...state, - count: action.payload.count, - }; - case types.COMPOSE_UPDATED_STATUS: - return { - ...state, - byId: { - ...state.byId, - [action.payload.id]: { - ...state.byId[action.payload.id], - image_status: action.payload.status, - }, - }, - }; - default: - return state; - } -} - -export default composes; diff --git a/src/store/types.js b/src/store/types.js deleted file mode 100644 index d969ca9b..00000000 --- a/src/store/types.js +++ /dev/null @@ -1,29 +0,0 @@ -const COMPOSE_ADDED = 'COMPOSE_ADDED'; -const COMPOSE_FAILED = 'COMPOSE_FAILED'; -const COMPOSE_UPDATED = 'COMPOSE_UPDATED'; -const COMPOSES_UPDATED_COUNT = 'COMPOSES_UPDATED_COUNT'; -const COMPOSE_UPDATED_STATUS = 'COMPOSE_UPDATED_STATUS'; -const SET_RELEASE = 'SET_RELEASE'; -const SET_UPLOAD_DESTINATIONS = 'SET_UPLOAD_DESTINATIONS'; -const SET_UPLOAD_AWS = 'SET_UPLOAD_AWS'; -const SET_UPLOAD_AZURE = 'SET_UPLOAD_AZURE'; -const SET_UPLOAD_GOOGLE = 'SET_UPLOAD_GOOGLE'; -const SET_SELECTED_PACKAGES = 'SET_SELECTED_PACKAGES'; -const SET_SUBSCRIPTION = 'SET_SUBSCRIPTION'; -const SET_SUBSCRIBE_NOW = 'SET_SUBSCRIBE_NOW'; - -export default { - COMPOSE_ADDED, - COMPOSE_FAILED, - COMPOSE_UPDATED, - COMPOSES_UPDATED_COUNT, - COMPOSE_UPDATED_STATUS, - SET_RELEASE, - SET_UPLOAD_DESTINATIONS, - SET_UPLOAD_AWS, - SET_UPLOAD_AZURE, - SET_UPLOAD_GOOGLE, - SET_SELECTED_PACKAGES, - SET_SUBSCRIPTION, - SET_SUBSCRIBE_NOW, -}; diff --git a/src/test/Components/LandingPage/LandingPage.test.js b/src/test/Components/LandingPage/LandingPage.test.js index fc6d3fd6..19a7a5f2 100644 --- a/src/test/Components/LandingPage/LandingPage.test.js +++ b/src/test/Components/LandingPage/LandingPage.test.js @@ -6,8 +6,8 @@ import api from '../../../api.js'; jest.mock('../../../store/actions/actions', () => { return { - composesGet: () => ({ type: 'foo' }), - composeGetStatus: () => ({ type: 'bar' }), + fetchComposes: () => ({ type: 'foo' }), + fetchComposeStatus: () => ({ type: 'bar' }), }; }); diff --git a/src/test/redux/actions.test.js b/src/test/redux/actions.test.js deleted file mode 100644 index 0d5717d3..00000000 --- a/src/test/redux/actions.test.js +++ /dev/null @@ -1,34 +0,0 @@ -import { actions } from '../../store/actions'; -import types from '../../store/types'; -import { RHEL_8 } from '../../constants.js'; - -const compose = { - '77e4c693-0497-4b85-936d-b2a3ad69571b': { - id: '77e4c693-0497-4b85-936d-b2a3ad69571b', - distribution: RHEL_8, - image_requests: [ - { - architecture: 'x86_64', - image_type: 'ami', - upload_request: { - type: 'aws', - options: {}, - }, - }, - ], - image_status: { - status: 'uploading', - }, - }, -}; - -describe('composeUpdated', () => { - test('returns dict', () => { - const result = actions.composeUpdated(compose); - - // this function updates the type attribute and - // returns everything else unchanged - expect(result.type).toBe(types.COMPOSE_UPDATED); - expect(result.payload.compose).toBe(compose); - }); -}); diff --git a/src/test/redux/reducers.test.js b/src/test/redux/reducers.test.js index ab870d6a..8c6d6ef2 100644 --- a/src/test/redux/reducers.test.js +++ b/src/test/redux/reducers.test.js @@ -1,5 +1,4 @@ -import { composes } from '../../store/reducers/composes'; -import types from '../../store/types'; +import composesSlice from '../../store/composesSlice'; import { RHEL_8 } from '../../constants.js'; const compose = { @@ -22,7 +21,7 @@ const compose = { describe('composes', () => { test('returns state for unknown actions', () => { - const result = composes( + const result = composesSlice( {}, { type: 'THIS-IS-UNKNOWN', @@ -32,15 +31,15 @@ describe('composes', () => { expect(result).toEqual({}); }); - test('returns updated state for types.COMPOSE_ADDED', () => { + test('returns updated state for composes/composeAdded', () => { const state = { allIds: [], byId: {}, count: 1, errors: null, }; - const result = composes(state, { - type: types.COMPOSE_ADDED, + const result = composesSlice(state, { + type: 'composes/composeAdded', payload: { compose }, }); @@ -52,44 +51,7 @@ describe('composes', () => { expect(result.error).toEqual(null); }); - test('returns updated state for types.COMPOSE_UPDATED', () => { - const state = { - allIds: ['77e4c693-0497-4b85-936d-b2a3ad69571b'], - byId: { - '77e4c693-0497-4b85-936d-b2a3ad69571b': {}, - }, - count: 2, - error: null, - }; - const result = composes(state, { - type: types.COMPOSE_UPDATED, - payload: { compose }, - }); - - expect(result.allIds).toEqual(['77e4c693-0497-4b85-936d-b2a3ad69571b']); - expect(result.byId['77e4c693-0497-4b85-936d-b2a3ad69571b']).toEqual( - compose - ); - expect(result.count).toEqual(2); - expect(result.error).toEqual(null); - }); - - test('returns updated state for types.COMPOSE_FAILED', () => { - const state = { - allIds: [], - byId: {}, - count: 0, - error: null, - }; - const result = composes(state, { - type: types.COMPOSE_FAILED, - payload: { error: 'test error' }, - }); - - expect(result.error).toEqual('test error'); - }); - - test('returns updated state for types.COMPOSES_UPDATED_COUNT', () => { + test('returns updated state for composes/composesUpdatedCount', () => { const state = { allIds: [], byId: {}, @@ -97,8 +59,8 @@ describe('composes', () => { error: null, }; - const result = composes(state, { - type: types.COMPOSES_UPDATED_COUNT, + const result = composesSlice(state, { + type: 'composes/composesUpdatedCount', payload: { count: 1 }, });