Skip to content

Commit

Permalink
[ML] Transform: Use redux toolkit for state management for edit trans…
Browse files Browse the repository at this point in the history
…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)
  • Loading branch information
walterra authored Jan 17, 2024
1 parent 4369ce9 commit 73c1bc0
Show file tree
Hide file tree
Showing 34 changed files with 984 additions and 711 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
import { frequencyValidator } from './frequency_validator';

describe('Transform: frequencyValidator()', () => {
// frequencyValidator() returns an array of error messages so
// an array with a length of 0 means a successful validation.

it('should fail when the input is not an integer and valid time unit.', () => {
expect(frequencyValidator('0')).toEqual(['The frequency value is not valid.']);
expect(frequencyValidator('0.1s')).toEqual(['The frequency value is not valid.']);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { type FC } from 'react';

import { EuiCallOut, EuiSpacer } from '@elastic/eui';

import { i18n } from '@kbn/i18n';

import { useApiErrorMessage } from '../state_management/selectors/api_error_message';

export const EditTransformApiErrorCallout: FC = () => {
const apiErrorMessage = useApiErrorMessage();

if (apiErrorMessage === undefined) return null;

return (
<>
<EuiSpacer size="m" />
<EuiCallOut
title={i18n.translate('xpack.transform.transformList.editTransformGenericErrorMessage', {
defaultMessage: 'An error occurred calling the API endpoint to update transforms.',
})}
color="danger"
iconType="warning"
>
<p>{apiErrorMessage}</p>
</EuiCallOut>
</>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,16 @@ import {
EuiTitle,
} from '@elastic/eui';

import { isManagedTransform } from '../../../../common/managed_transforms_utils';
import { isManagedTransform } from '../../../common/managed_transforms_utils';

import { ManagedTransformsWarningCallout } from '../managed_transforms_callout/managed_transforms_callout';
import type { EditAction } from '../action_edit';
import { ManagedTransformsWarningCallout } from '../../transform_management/components/managed_transforms_callout/managed_transforms_callout';
import type { EditAction } from '../../transform_management/components/action_edit';

import { EditTransformFlyoutProvider } from '../state_management/edit_transform_flyout_state';

import { EditTransformApiErrorCallout } from './edit_transform_api_error_callout';
import { EditTransformFlyoutCallout } from './edit_transform_flyout_callout';
import { EditTransformFlyoutForm } from './edit_transform_flyout_form';
import { EditTransformFlyoutProvider } from './use_edit_transform_flyout';
import { EditTransformUpdateButton } from './edit_transform_update_button';

export const EditTransformFlyout: FC<EditAction> = ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
EuiTextColor,
} from '@elastic/eui';

import { useDocumentationLinks } from '../../../../hooks/use_documentation_links';
import { useDocumentationLinks } from '../../../hooks/use_documentation_links';
export const EditTransformFlyoutCallout: FC = () => {
const { esTransformUpdate } = useDocumentationLinks();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@ import { EuiFormRow, EuiTextArea } from '@elastic/eui';

import { i18n } from '@kbn/i18n';

import {
useEditTransformFlyout,
type EditTransformHookTextInputSelectors,
} from './use_edit_transform_flyout';
import { capitalizeFirstLetter } from './capitalize_first_letter';
import { useEditTransformFlyoutActions } from '../state_management/edit_transform_flyout_state';
import { useFormField } from '../state_management/selectors/form_field';
import type { FormFields } from '../state_management/form_field';
import { capitalizeFirstLetter } from '../utils/capitalize_first_letter';

interface EditTransformFlyoutFormTextInputProps {
field: EditTransformHookTextInputSelectors;
field: FormFields;
label: string;
helpText?: string;
placeHolder?: boolean;
Expand All @@ -30,8 +29,8 @@ export const EditTransformFlyoutFormTextArea: FC<EditTransformFlyoutFormTextInpu
helpText,
placeHolder = false,
}) => {
const { defaultValue, errorMessages, value } = useEditTransformFlyout(field);
const { formField } = useEditTransformFlyout('actions');
const { defaultValue, errorMessages, value } = useFormField(field);
const { setFormField } = useEditTransformFlyoutActions();
const upperCaseField = capitalizeFirstLetter(field);

return (
Expand All @@ -53,7 +52,7 @@ export const EditTransformFlyoutFormTextArea: FC<EditTransformFlyoutFormTextInpu
}
isInvalid={errorMessages.length > 0}
value={value}
onChange={(e) => formField({ field, value: e.target.value })}
onChange={(e) => setFormField({ field, value: e.target.value })}
aria-label={label}
/>
</EuiFormRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@ import { EuiFieldText, EuiFormRow } from '@elastic/eui';

import { i18n } from '@kbn/i18n';

import {
useEditTransformFlyout,
type EditTransformHookTextInputSelectors,
} from './use_edit_transform_flyout';
import { capitalizeFirstLetter } from './capitalize_first_letter';
import { useEditTransformFlyoutActions } from '../state_management/edit_transform_flyout_state';
import { useFormField } from '../state_management/selectors/form_field';
import type { FormFields } from '../state_management/form_field';
import { capitalizeFirstLetter } from '../utils/capitalize_first_letter';

interface EditTransformFlyoutFormTextInputProps {
field: EditTransformHookTextInputSelectors;
field: FormFields;
label: string;
helpText?: string;
placeHolder?: boolean;
Expand All @@ -30,8 +29,8 @@ export const EditTransformFlyoutFormTextInput: FC<EditTransformFlyoutFormTextInp
helpText,
placeHolder = false,
}) => {
const { defaultValue, errorMessages, value } = useEditTransformFlyout(field);
const { formField } = useEditTransformFlyout('actions');
const { defaultValue, errorMessages, value } = useFormField(field);
const { setFormField } = useEditTransformFlyoutActions();
const upperCaseField = capitalizeFirstLetter(field);

return (
Expand All @@ -53,7 +52,7 @@ export const EditTransformFlyoutFormTextInput: FC<EditTransformFlyoutFormTextInp
}
isInvalid={errorMessages.length > 0}
value={value}
onChange={(e) => formField({ field, value: e.target.value })}
onChange={(e) => setFormField({ field, value: e.target.value })}
aria-label={label}
/>
</EuiFormRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@ import { useEuiTheme, EuiComboBox, EuiFormRow, EuiSkeletonRectangle } from '@ela

import { i18n } from '@kbn/i18n';

import { useGetEsIngestPipelines } from '../../../../hooks';
import { useGetEsIngestPipelines } from '../../../hooks';

import { useEditTransformFlyoutActions } from '../state_management/edit_transform_flyout_state';
import { useFormField } from '../state_management/selectors/form_field';

import { EditTransformFlyoutFormTextInput } from './edit_transform_flyout_form_text_input';
import { useEditTransformFlyout } from './use_edit_transform_flyout';

const ingestPipelineLabel = i18n.translate(
'xpack.transform.transformList.editFlyoutFormDestinationIngestPipelineLabel',
Expand All @@ -25,8 +27,8 @@ const ingestPipelineLabel = i18n.translate(

export const EditTransformIngestPipeline: FC = () => {
const { euiTheme } = useEuiTheme();
const { errorMessages, value } = useEditTransformFlyout('destinationIngestPipeline');
const { formField } = useEditTransformFlyout('actions');
const { errorMessages, value } = useFormField('destinationIngestPipeline');
const { setFormField } = useEditTransformFlyoutActions();

const { data: esIngestPipelinesData, isLoading } = useGetEsIngestPipelines();
const ingestPipelineNames = esIngestPipelinesData?.map(({ name }) => name) ?? [];
Expand Down Expand Up @@ -66,7 +68,7 @@ export const EditTransformIngestPipeline: FC = () => {
options={ingestPipelineNames.map((label: string) => ({ label }))}
selectedOptions={[{ label: value }]}
onChange={(o) =>
formField({ field: 'destinationIngestPipeline', value: o[0]?.label ?? '' })
setFormField({ field: 'destinationIngestPipeline', value: o[0]?.label ?? '' })
}
/>
</EuiSkeletonRectangle>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,45 @@

import React, { useEffect, useMemo, type FC } from 'react';

import { i18n } from '@kbn/i18n';

import { EuiFormRow, EuiSelect, EuiSpacer, EuiSwitch } from '@elastic/eui';

import { i18n } from '@kbn/i18n';
import { toMountPoint } from '@kbn/react-kibana-mount';
import { useAppDependencies, useToastNotifications } from '../../../../app_dependencies';
import { ToastNotificationText } from '../../../../components';
import { isLatestTransform, isPivotTransform } from '../../../../../../common/types/transform';
import { useGetTransformsPreview } from '../../../../hooks';

import type { PostTransformsPreviewRequestSchema } from '../../../../../common/api_schemas/transforms';
import { isLatestTransform, isPivotTransform } from '../../../../../common/types/transform';
import { getErrorMessage } from '../../../../../common/utils/errors';

import { useAppDependencies, useToastNotifications } from '../../../app_dependencies';
import { useGetTransformsPreview } from '../../../hooks';
import { ToastNotificationText } from '../../../components';

import {
useEditTransformFlyoutActions,
useEditTransformFlyoutContext,
} from '../state_management/edit_transform_flyout_state';
import { useFormSections } from '../state_management/selectors/form_sections';
import { useRetentionPolicyField } from '../state_management/selectors/retention_policy_field';

import { EditTransformFlyoutFormTextInput } from './edit_transform_flyout_form_text_input';
import { useEditTransformFlyout } from './use_edit_transform_flyout';
import { getErrorMessage } from '../../../../../../common/utils/errors';

export const EditTransformRetentionPolicy: FC = () => {
const { i18n: i18nStart, theme } = useAppDependencies();

const toastNotifications = useToastNotifications();

const dataViewId = useEditTransformFlyout('dataViewId');
const formSections = useEditTransformFlyout('stateFormSection');
const retentionPolicyField = useEditTransformFlyout('retentionPolicyField');
const { formField, formSection } = useEditTransformFlyout('actions');
const requestConfig = useEditTransformFlyout('config');
const { config, dataViewId } = useEditTransformFlyoutContext();
const formSections = useFormSections();
const retentionPolicyField = useRetentionPolicyField();
const { setFormField, setFormSection } = useEditTransformFlyoutActions();

const previewRequest = useMemo(() => {
const previewRequest: PostTransformsPreviewRequestSchema = useMemo(() => {
return {
source: requestConfig.source,
...(isPivotTransform(requestConfig) ? { pivot: requestConfig.pivot } : {}),
...(isLatestTransform(requestConfig) ? { latest: requestConfig.latest } : {}),
source: config.source,
...(isPivotTransform(config) ? { pivot: config.pivot } : {}),
...(isLatestTransform(config) ? { latest: config.latest } : {}),
};
}, [requestConfig]);
}, [config]);

const { error: transformsPreviewError, data: transformPreview } =
useGetTransformsPreview(previewRequest);
Expand Down Expand Up @@ -98,7 +106,7 @@ export const EditTransformRetentionPolicy: FC = () => {
)}
checked={formSections.retentionPolicy.enabled}
onChange={(e) =>
formSection({
setFormSection({
section: 'retentionPolicy',
enabled: e.target.checked,
})
Expand Down Expand Up @@ -142,7 +150,7 @@ export const EditTransformRetentionPolicy: FC = () => {
options={retentionDateFieldOptions}
value={retentionPolicyField.value}
onChange={(e) =>
formField({ field: 'retentionPolicyField', value: e.target.value })
setFormField({ field: 'retentionPolicyField', value: e.target.value })
}
hasNoInitialSelection={
!retentionDateFieldOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,38 @@ import { EuiButton } from '@elastic/eui';

import { i18n } from '@kbn/i18n';

import { getErrorMessage } from '../../../../../../common/utils/errors';
import { getErrorMessage } from '../../../../../common/utils/errors';

import { useUpdateTransform } from '../../../../hooks';
import { useUpdateTransform } from '../../../hooks';

import { useEditTransformFlyout } from './use_edit_transform_flyout';
import {
useEditTransformFlyoutActions,
useEditTransformFlyoutContext,
} from '../state_management/edit_transform_flyout_state';
import { useIsFormTouched } from '../state_management/selectors/is_form_touched';
import { useIsFormValid } from '../state_management/selectors/is_form_valid';
import { useUpdatedTransformConfig } from '../state_management/selectors/updated_transform_config';

interface EditTransformUpdateButtonProps {
closeFlyout: () => void;
}

export const EditTransformUpdateButton: FC<EditTransformUpdateButtonProps> = ({ closeFlyout }) => {
const requestConfig = useEditTransformFlyout('requestConfig');
const isUpdateButtonDisabled = useEditTransformFlyout('isUpdateButtonDisabled');
const config = useEditTransformFlyout('config');
const { apiError } = useEditTransformFlyout('actions');
const { config } = useEditTransformFlyoutContext();
const isFormValid = useIsFormValid();
const isFormTouched = useIsFormTouched();
const requestConfig = useUpdatedTransformConfig();
const isUpdateButtonDisabled = !isFormValid || !isFormTouched;

const { setApiError } = useEditTransformFlyoutActions();

const updateTransfrom = useUpdateTransform(config.id, requestConfig);

async function submitFormHandler() {
apiError(undefined);
setApiError(undefined);

updateTransfrom(undefined, {
onError: (error) => apiError(getErrorMessage(error)),
onError: (error) => setApiError(getErrorMessage(error)),
onSuccess: () => closeFlyout(),
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
* 2.0.
*/

export { EditTransformFlyout } from './edit_transform_flyout';
export { EditTransformFlyout } from './components/edit_transform_flyout';
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { TransformPivotConfig } from '../../../../../../common/types/transform';

export const getTransformConfigMock = (): TransformPivotConfig => ({
id: 'the-transform-id',
source: {
index: ['the-transform-source-index'],
query: {
match_all: {},
},
},
dest: {
index: 'the-transform-destination-index',
},
pivot: {
group_by: {
airline: {
terms: {
field: 'airline',
},
},
},
aggregations: {
'responsetime.avg': {
avg: {
field: 'responsetime',
},
},
},
},
description: 'the-description',
});
Loading

0 comments on commit 73c1bc0

Please sign in to comment.