Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
EnhancedStore,
Reducer,
ReducersMapObject,
ThunkDispatch,
UnknownAction,
} from '@reduxjs/toolkit';
import { AxiosInstance } from 'axios';
Expand Down Expand Up @@ -29,14 +30,15 @@ export type MountedReducers = OptionalRecord<StateSchemaKey, boolean>;

export interface ReducerManager {
getReducerMap: () => ReducersMapObject<StateSchema>;
reduce: (state: StateSchema, action: UnknownAction) => StateSchema;
reduce: Reducer<StateSchema>;
add: (key: StateSchemaKey, reducer: Reducer) => void;
remove: (key: StateSchemaKey) => void;
getMountedReducers: () => MountedReducers;
}

export interface ReduxStoreWithManager extends EnhancedStore<StateSchema> {
reducerManager: ReducerManager;
dispatch: ThunkDispatch<StateSchema, ThunkExtraArg, UnknownAction>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we want to add this type definition?

As far as I see, we're still using type assertion with "unknown assertion" first (as unknown as ReduxStoreWithManager), so can we say that the issue is actually fixed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the issue is actually fixed because we use a cast once in the "factory" (createReduxStore) so that entire codebase can be clean and type-safe.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd say that cast is a problematic solution by itself and we still need to have a TODO or FIXME comment if we're going to rely on the cast...

Is it possible to resolve typecheck errors without a cast in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point, the cast is a workaround since StateSchema has required properties but delete removes them at runtime. Making keys optional would fix it properly but adds null checks everywhere across selectors and components.
Adding a FIXME comment to flag the debt as suggested - let me know if you'd prefer a different approach.

}

export interface ThunkExtraArg {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,15 @@ export function createReducerManager(
return {
getReducerMap: () => reducers,
getMountedReducers: () => mountedReducers,
reduce: (state: StateSchema, action: UnknownAction) => {
if (keysToRemove.length > 0) {
reduce: (state: StateSchema | undefined, action: UnknownAction) => {
if (state && keysToRemove.length > 0) {
state = { ...state };
keysToRemove.forEach((key) => {
// TODO: fix types
// @ts-ignore
delete state[key];
delete (state as Partial<StateSchema>)[key];
});
keysToRemove = [];
}

// @ts-ignore
return combinedReducer(state, action);
},
add: (key: StateSchemaKey, reducer: Reducer) => {
Expand All @@ -54,9 +51,7 @@ export function createReducerManager(
return;
}

// TODO: fix types
// @ts-ignore
delete reducers[key];
delete (reducers as Partial<ReducersMapObject<StateSchema>>)[key];
keysToRemove.push(key);

mountedReducers[key] = false;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { configureStore, ReducersMapObject } from '@reduxjs/toolkit';
import { Reducer } from 'redux';

Check warning on line 2 in heka-identity-service-web-ui/src/app/providers/StoreProvider/config/store.ts

View workflow job for this annotation

GitHub Actions / Build and check

'Reducer' is defined but never used

import { connectionReducer } from '@/entities/Connection';
import { credentialReducer } from '@/entities/Credential';
Expand All @@ -12,7 +12,7 @@
import { $agencyApi, $authApi } from '@/shared/api/config/api';

import { createReducerManager } from './reducerManager';
import { StateSchema, ThunkExtraArg } from './StateSchema';
import { ReduxStoreWithManager, StateSchema, ThunkExtraArg } from './StateSchema';

export function createReduxStore(
initialState?: StateSchema,
Expand All @@ -38,7 +38,7 @@
};

const store = configureStore({
reducer: reducerManager.reduce as Reducer<StateSchema>,
reducer: reducerManager.reduce,
devTools: __IS_DEV__,
preloadedState: initialState,
middleware: (getDefaultMiddleware) =>
Expand All @@ -47,9 +47,8 @@
extraArgument: extraArg,
},
}),
});
}) as unknown as ReduxStoreWithManager;

// @ts-ignore
store.reducerManager = reducerManager;

return store;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ interface DynamicModuleLoaderProps {
export const DynamicModuleLoader = (props: DynamicModuleLoaderProps) => {
const { children, reducers, removeAfterUnmount = true } = props;

const store = useStore() as ReduxStoreWithManager;
const store = useStore() as unknown as ReduxStoreWithManager;
const dispatch = useDispatch();

useEffect(() => {
Expand Down
Loading