Skip to content
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

[ML] Transform: Use redux toolkit for state management for edit transform flyout #173861

Merged
merged 38 commits into from
Jan 17, 2024

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Dec 21, 2023

Summary

Part of #151664.

This replaces the state management of the edit transform flyout with Redux Toolkit (which is already part of Kibana's package.json). Previously, we avoided redux because of the additional boilerplate. However, Redux Toolkit provides utilities (e.g. createSlice, immutable state updates via immer) that significantly reduce the necessary boilerplate. For example, there is no longer a need to write classic action creators or even a reducer. In fact, this PR gets rid of action reactors and the reducer that were mimicking classic redux behaviour. If you know a bit of old plain boilerplaty redux, have a look here how it looks with Redux Toolkit: https://redux-toolkit.js.org/tutorials/quick-start (look out for the counterSlice part, that explain the main difference to writing old school action creators and reducers).

So instead of a full reducer and corresponding action definitions, we can now write more simple callbacks that will end up as reducer actions being automatically set up via createSlice:

const setApiError = (state: State, action: PayloadAction<string | undefined>) => {
  state.apiErrorMessage = action.payload;
};

Note that under the hood redux toolkit uses immer which allows us to write the above shorter notation, it let's us treat immutable state updates as if we're mutating state. Otherwise we'd have to write the following to be returned from the action:

({
    ...state,
    apiErrorMessage: action.payload
})

This becomes even more useful for otherwise painful nested state updates. Here's a nice reference on how to do various types of state updates with immer: https://immerjs.github.io/immer/update-patterns/

On the other hand, to consume data from the redux store, we use so-called selectors. Under the hood they are optimized to avoid unnecessary rerenders or even render loops, something we especially had to work around in the state management of the transform creation wizard with custom state comparison. Simple selector setup and usage would look like this:

// state.ts
export const selectApiErrorMessage = (s: State) => s.apiErrorMessage;
export const useApiErrorMessage = () => useSelector(selectApiErrorMessage);

// component.tsx
export const ApiErrorCallout: FC = () => {
    const apiErrorMessage = useApiErrorMessage();
    return <p>{apiErrorMessage}</p>;
}

It's certainly possible and you might be tempted to write these simple selectors inline like useSelector((s: State) => s.apiErrorMessage). However, note that you'd then still have to pass around the State type. And you might quickly lose track of which state attributes you use across your components. Keeping the selector code close to where you manage state will help with maintainability and testing.

Be aware that as soon as you require local state in components derived from more than one redux store attribute or including more complex transformations, you might again run into unnecessary rerenders. To work around this, redux toolkit includes the reselect library's createSelector. This will allow you to write selectors with proper memoization. They work a bit like the map-reduce pattern: As the map part you'll select multiple attributes from your state and the reduce part will return data derived from these state attributes. Think of it as a map-reduce-like subscription to your store. For example, prior to this PR, we set the isFormValid attribute actively as part of the update actions in the form's state. This new version no longer has isFormValid as a state attribute, instead it is derived from the form's field statuses as part of a selector and we "subscribe" to it using useSelector().

// state.ts
const isFormValid = (formFields: FormFieldsState) =>
  Object.values(formFields).every((d) => d.errorMessages.length === 0);
const selectIsFormValid = createSelector((state: State) => state.formFields, isFormValid);
export const useIsFormValid = () => useSelector(selectIsFormValid);

// component.tsx
export const UpdateTransform: FC = () => {
    const isFormValid = useIsFormValid();
    ....
}

In the above code, the isFormValid() function in state.ts is the same code we used previously to actively verify on state actions. However, this approach was more risky because we could miss adding that check on a new state action. Instead, selectIsFormValid sets us up to switch to the more subscription like pattern. For createSelector, the first argument state: State) => state.formFields just picks formFields (= map step), the second argument passes isFormValid to do the actual validation (= reduce step). Finally, for more convenience we wrap everything in a custom hook useIsFormValid. This way the consuming component ends up really simple, all with proper memoization in place.

Memoization gets a bit more tricky if we want to combine selectors with information we have only available as props or via react context. For example, the wrapping component of the flyout to edit transforms provides the original transform config and an optional dataViewId. If we want to find out if a user changed the form, we need to compare the form state to the original transform config. The following code sets us up to achieve that with memoization:

// state.ts
const createSelectIsFormTouched = (originalConfig: TransformConfigUnion) =>
  createSelector(
    (state: State) => state.formFields,
    (state: State) => state.formSections,
    (formFields, formSections) => isFormTouched(originalConfig, formFields, formSections)
  );
export const useIsFormTouched = () => {
  const { config } = useEditTransformFlyoutContext();
  const selectIsFormTouched = useMemo(() => createSelectIsFormTouched(config), [config]);
  return useSelector(selectIsFormTouched);
};

// component.tsx
export const UpdateTransform: FC = () => {
    const isFormTouched = useIsFormTouched();
    ....
}

createSelectIsFormTouched is a factory that takes the original transform config and returns a selector that uses it to verify if the form state changed from the original config (That's what's called currying: A function returning another function, where the args of the first function get set in stone and are available to the scope of the second function). To properly memoize this, the custom hook useIsFormTouched() puts this factory inside a useMemo so the selector would only change once the original config changes. Then that memoized selector gets passed on to useSelector. Again, the code in the component itself ends up being really simple. For more examples on how to write proper memoized selectors, have a look here: https://github.com/amsterdamharu/selectors

Checklist

@walterra walterra self-assigned this Dec 21, 2023
@walterra walterra force-pushed the ml-transform-edit-redux branch from 635d265 to 3f211d2 Compare December 21, 2023 19:13
@walterra walterra force-pushed the ml-transform-edit-redux branch 3 times, most recently from 5116854 to 2c87b05 Compare January 5, 2024 13:39
@walterra walterra marked this pull request as ready for review January 10, 2024 16:42
@walterra walterra requested a review from a team as a code owner January 10, 2024 16:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra walterra requested a review from qn895 January 11, 2024 17:51
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested with a variety of edits for batch and continuous transforms and LGTM


import type { State } from '../edit_transform_flyout_state';

export const selectRetentionPolicyField = (s: State) => s.formFields.retentionPolicyField;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Doesn't look like we are using selectRetentionPolicyField outside of this file. No need to export.

applyFormStateToTransformConfig(originalConfig, formFields, formSections)
);

export const useUpdatedTransformConfig = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thought but not blocking: Might be more descriptive to name the folder hooks (or hooks and selectors (?)) instead of selectors since that's what we are mainly exporting. Whether we are using selectors under the hood is an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going back and forth regarding this too. I'll leave it as is for now but will revisit in a follow up.

// Takes a value from form state and applies it to the structure
// of the expected final configuration request object.
// Considers options like if a value is nullable or optional.
export const getUpdateValue = (
Copy link
Member

Choose a reason for hiding this comment

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

For future follow ups: Would be good to have unit tests for this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have dedicated unit tests for this file yet but applyFormStateToTransformConfig() exclusively makes use of this function and is covered by unit tests. I plan to move the code related to these form utilities to a package and will add more tests as part of that move. Updated the issue in that regard tracking the redux migration: #151664

@qn895
Copy link
Member

qn895 commented Jan 16, 2024

Tested and LGTM other than the non-blocking nit comments.

const store = useMemo(getReduxStore, []);

// Apply original transform config to redux form state.
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: useMount, so it's clear the callback should invoke only on mount and we can omit // eslint-disable-next-line react-hooks/exhaustive-deps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated in 31d5d15.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
transform 405 421 +16

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
transform 402.9KB 402.5KB -444.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
transform 18.2KB 18.4KB +161.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

@walterra walterra merged commit 73c1bc0 into elastic:main Jan 17, 2024
16 of 17 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 17, 2024
@walterra walterra deleted the ml-transform-edit-redux branch January 17, 2024 13:42
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
…form flyout (elastic#173861)

## Summary

Part of elastic#151664.

This replaces the state management of the edit transform flyout with
Redux Toolkit (which is already part of Kibana's `package.json`).
Previously, we avoided redux because of the additional boilerplate.
However, Redux Toolkit provides utilities (e.g. `createSlice`, immutable
state updates via `immer`) that significantly reduce the necessary
boilerplate. For example, there is no longer a need to write classic
action creators or even a reducer. In fact, this PR gets rid of action
reactors and the reducer that were mimicking classic redux behaviour. If
you know a bit of old plain boilerplaty redux, have a look here how it
looks with Redux Toolkit:
https://redux-toolkit.js.org/tutorials/quick-start (look out for the
`counterSlice` part, that explain the main difference to writing old
school action creators and reducers).

So instead of a full reducer and corresponding action definitions, we
can now write more simple callbacks that will end up as reducer actions
being automatically set up via `createSlice`:

```ts
const setApiError = (state: State, action: PayloadAction<string | undefined>) => {
  state.apiErrorMessage = action.payload;
};
```

Note that under the hood redux toolkit uses `immer` which allows us to
write the above shorter notation, it let's us treat immutable state
updates as if we're mutating `state`. Otherwise we'd have to write the
following to be returned from the action:

```ts
({
    ...state,
    apiErrorMessage: action.payload
})
```

This becomes even more useful for otherwise painful nested state
updates. Here's a nice reference on how to do various types of state
updates with `immer`: https://immerjs.github.io/immer/update-patterns/

On the other hand, to consume data from the redux store, we use
so-called selectors. Under the hood they are optimized to avoid
unnecessary rerenders or even render loops, something we especially had
to work around in the state management of the transform creation wizard
with custom state comparison. Simple selector setup and usage would look
like this:

```ts
// state.ts
export const selectApiErrorMessage = (s: State) => s.apiErrorMessage;
export const useApiErrorMessage = () => useSelector(selectApiErrorMessage);

// component.tsx
export const ApiErrorCallout: FC = () => {
    const apiErrorMessage = useApiErrorMessage();
    return <p>{apiErrorMessage}</p>;
}
```

It's certainly possible and you might be tempted to write these simple
selectors inline like `useSelector((s: State) => s.apiErrorMessage)`.
However, note that you'd then still have to pass around the `State`
type. And you might quickly lose track of which state attributes you use
across your components. Keeping the selector code close to where you
manage state will help with maintainability and testing.

Be aware that as soon as you require local state in components derived
from more than one redux store attribute or including more complex
transformations, you might again run into unnecessary rerenders. To work
around this, redux toolkit includes the `reselect` library's
`createSelector`. This will allow you to write selectors with proper
memoization. They work a bit like the map-reduce pattern: As the `map`
part you'll select multiple attributes from your state and the `reduce`
part will return data derived from these state attributes. Think of it
as a map-reduce-like subscription to your store. For example, prior to
this PR, we set the `isFormValid` attribute actively as part of the
update actions in the form's state. This new version no longer has
`isFormValid` as a state attribute, instead it is derived from the
form's field statuses as part of a selector and we "subscribe" to it
using `useSelector()`.

```ts
// state.ts
const isFormValid = (formFields: FormFieldsState) =>
  Object.values(formFields).every((d) => d.errorMessages.length === 0);
const selectIsFormValid = createSelector((state: State) => state.formFields, isFormValid);
export const useIsFormValid = () => useSelector(selectIsFormValid);

// component.tsx
export const UpdateTransform: FC = () => {
    const isFormValid = useIsFormValid();
    ....
}
```

In the above code, the `isFormValid()` function in `state.ts` is the
same code we used previously to actively verify on state actions.
However, this approach was more risky because we could miss adding that
check on a new state action. Instead, `selectIsFormValid` sets us up to
switch to the more subscription like pattern. For `createSelector`, the
first argument `state: State) => state.formFields` just picks
`formFields` (= map step), the second argument passes `isFormValid` to
do the actual validation (= reduce step). Finally, for more convenience
we wrap everything in a custom hook `useIsFormValid`. This way the
consuming component ends up really simple, all with proper memoization
in place.

Memoization gets a bit more tricky if we want to combine selectors with
information we have only available as props or via react context. For
example, the wrapping component of the flyout to edit transforms
provides the original transform `config` and an optional `dataViewId`.
If we want to find out if a user changed the form, we need to compare
the form state to the original transform config. The following code sets
us up to achieve that with memoization:

```ts
// state.ts
const createSelectIsFormTouched = (originalConfig: TransformConfigUnion) =>
  createSelector(
    (state: State) => state.formFields,
    (state: State) => state.formSections,
    (formFields, formSections) => isFormTouched(originalConfig, formFields, formSections)
  );
export const useIsFormTouched = () => {
  const { config } = useEditTransformFlyoutContext();
  const selectIsFormTouched = useMemo(() => createSelectIsFormTouched(config), [config]);
  return useSelector(selectIsFormTouched);
};

// component.tsx
export const UpdateTransform: FC = () => {
    const isFormTouched = useIsFormTouched();
    ....
}
```

`createSelectIsFormTouched` is a factory that takes the original
transform config and returns a selector that uses it to verify if the
form state changed from the original config (That's what's called
currying: A function returning another function, where the args of the
first function get set in stone and are available to the scope of the
second function). To properly memoize this, the custom hook
`useIsFormTouched()` puts this factory inside a `useMemo` so the
selector would only change once the original config changes. Then that
memoized selector gets passed on to `useSelector`. Again, the code in
the component itself ends up being really simple. For more examples on
how to write proper memoized selectors, have a look here:
https://github.com/amsterdamharu/selectors

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Transforms ML transforms :ml release_note:skip Skip the PR/issue when compiling release notes v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants