clean up indexes store and pass values directly #4821
Conversation
| const rootReducer = (state: RootState, action: AnyAction): RootState => { | ||
| if (action.type === RESET_FORM) { | ||
| return { | ||
| ...state, | ||
| isWritable: WRITABLE_INITIAL_STATE, | ||
| isReadonlyView: READONLY_VIEW_INITIAL_STATE, | ||
| description: DESCRIPTION_INITIAL_STATE, | ||
| serverVersion: SV_INITIAL_STATE, | ||
| namespace: NAMESPACE_INITIAL_STATE, | ||
| regularIndexes: REGULAR_INDEXES_INITIAL_STATE, | ||
| searchIndexes: SEARCH_INDEXES_INITIAL_STATE, | ||
| }; | ||
| } | ||
| return reducer(state, action); | ||
| }; |
There was a problem hiding this comment.
Removed this as we do not have any form fields in view-indexes. This clean up is only applicable to create/drop indexes
There was a problem hiding this comment.
Awesome, those RESET actions are the biggest mess in old reducers in Compass, nice that we can remove it so easily
| } | ||
| } | ||
|
|
||
| void store.dispatch(fetchIndexes()); |
There was a problem hiding this comment.
This probably should be triggered from the UI, but keeping it here as was.
There was a problem hiding this comment.
Yeah, when you are doing similar for search indexes, would probably be good to move this to UI too
mabaasit
changed the title
| const localAppRegistry = options.localAppRegistry; | ||
| store.dispatch(localAppRegistryActivated(localAppRegistry)); |
There was a problem hiding this comment.
If those are not in the store, we don't need to dispatch this, same for global one
There was a problem hiding this comment.
we probably need it because we trigger delete/create modals using localAppRegistry. And by removing this, it not able to find the registry and throws.
There was a problem hiding this comment.
Yes, drop index and create index stores both have app registry slice, but this is a completely different store, you literally don't have a slice here that would react to those actions
There was a problem hiding this comment.
Wait i missed adding the registry. We have a default slice from the redux-common which is used in this store and then used on the UI to trigger actions.
I added this commit to the parent branch 91195c6
| } | ||
| } | ||
|
|
||
| void store.dispatch(fetchIndexes()); |
There was a problem hiding this comment.
Yeah, when you are doing similar for search indexes, would probably be good to move this to UI too
| @@ -247,7 +250,8 @@ export const fetchIndexes = (): ThunkAction< | |||
| } = getState(); | |||
|
|
|||
| if (isReadonlyView) { | |||
| return _handleIndexesChanged(dispatch, []); | |||
| dispatch(_handleIndexesChanged([])); | |||
| store.dispatch(setIndexes(defaultSortedDesc as any)); | ||
|
|
||
| { | ||
| await store.dispatch(sortIndexes('Name and Definition', 'asc')); | ||
| store.dispatch(sortIndexes('Name and Definition', 'asc') as any); |
There was a problem hiding this comment.
You need to assert here because Store<RootState> above is not a correct type for the store (it doesn't take thunk into account), if you change it to ReturnType<typeof setupStore> the need for this should go away (and this actually validates that the thunk type is matching what we are passing as extra args, so a good thing to have)
There was a problem hiding this comment.
oh that's awesome :) i spent time figuring this out and ended up with any.
There was a problem hiding this comment.
One cool (but totally not obvious thing) about redux middleware (not only thunk one) types is that it can actually change the type of store and dispatch methods, very handy technique
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes