refactor: Rework undo to typescript #44479
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 674ac97...825ea9c.
|
|
frontend/src/metabase/redux/undo.ts
Outdated
| @@ -16,12 +19,18 @@ export const addUndo = createThunkAction(ADD_UNDO, undo => { | |||
| const { icon = "check", timeout = 5000, canDismiss = true } = undo; | |||
| const id = undo.id ?? nextUndoId++; | |||
| // if we're overwriting an existing undo, clear its timeout | |||
| const currentUndo = getUndo(getState(), id); | |||
| clearTimeoutForUndo(currentUndo); | |||
| const currentUndo = getUndo(getState().undo, id); | |||
There was a problem hiding this comment.
Nit: getUndo is a selector and selectors usually receive the entire state
frontend/src/metabase/redux/undo.ts
Outdated
| if (undo.timeoutId) { | ||
| clearTimeout(undo.timeoutId); | ||
| } |
There was a problem hiding this comment.
We can use clearTimeoutForUndo here
frontend/src/metabase/redux/undo.ts
Outdated
| @@ -202,7 +214,7 @@ export default function (state = [], { type, payload, error }) { | |||
| return state; | |||
| } | |||
|
|
|||
| const clearTimeoutForUndo = undo => { | |||
| const clearTimeoutForUndo = (undo: Undo) => { | |||
| if (undo?.timeoutId) { | |||
There was a problem hiding this comment.
We can remove ? now or we can allow undefined to be passed here and then we don't have to call this function conditionally anymore
frontend/src/metabase/redux/undo.ts
Outdated
| export default function (state = [], { type, payload, error }) { | ||
| export function undoReducer( | ||
| state: Undo[] = [], | ||
| { type, payload, error }: Action<Undo>, |
There was a problem hiding this comment.
DISMISS_UNDO the payload will be Undo["id"].
| { type, payload, error }: Action<Undo>, | |
| { type, payload, error }: Action<Undo | Undo["id"]>, |
WDYT about adding 1 (isUndoId) or 2 (isUndoId + isUndo) type guards? So that this reducer can look like this:
if (type === ADD_UNDO && !isUndoId(payload) {
// ...
} else if (type === DISMISS_UNDO && isUndoId(payload) {
// ...
} else // ...or this:
if (type === ADD_UNDO && isUndo(payload) {
// ...
} else if (type === DISMISS_UNDO && isUndoId(payload) {
// ...
} else // ...And then we can remove the as unknown as Undo["id"] cast
There was a problem hiding this comment.
I tried it as well, good point. I came up to the idea that we'll need to rework this reducer to RTK anyway and builder.addCase supports custom payload type, so I decided not to waste time on this check
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
Closes #44433
Description
Add types to redux/undo and UndoListing
How to verify
Checklist