Redux types #63413
Redux types #63413
Conversation
| @@ -13,6 +13,8 @@ import { alertingIndexGetQuerySchema } from './schema/alert_index'; | |||
| */ | |||
| export type Immutable<T> = T extends undefined | null | boolean | string | number | |||
| ? T | |||
| : unknown extends T | |||
| ? unknown | |||
There was a problem hiding this comment.
this is safe because anything union'ed w/ unknown is unknown
There was a problem hiding this comment.
why do you need to do : unknown extends T ? unknown
There was a problem hiding this comment.
Immutable is meant to be able to accept anything. If you have a type such as:
type nerds = {
a: string,
b: unknown
}
Then when we recurse to b, the value will be Immutable<unknown>. Without special handling this will fallback to ImmutableObject<unknown>. ImmutableObject<unknown> doesn't work (maybe ImmutableObject should have stricter type parameter constraints.)
Therefore T extends unknown wouldn't be a good way to handle unknown as it will always be true. e.g.:
type itsTrue = 1 extends unknown ? true : false
If you have any union with unknown, it is the same as unknown. This is because that union can accept anything (everything is assignable to unknown.) So if we want to have Immutable<unknown> alias unknown, we can check if unknown can be assigned to T.
This use case arose because this PR uses Immutable on a data structure from the data plugin which has unknown as the value of a key of an object.
| @@ -142,7 +141,7 @@ export const searchBarIndexPatterns = (state: AlertListState) => state.searchBar | |||
| * query params to use when requesting alert data. | |||
| */ | |||
| export const apiQueryParams: ( | |||
| state: AlertListState | |||
| state: Immutable<AlertListState> | |||
There was a problem hiding this comment.
its not generally necessary to add Immutable everywhere, but it can be useful when debugging. i may or may not remove these. they dont hurt anything
| @@ -21,7 +20,7 @@ const initialState = (): HostListState => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const hostListReducer: Reducer<HostListState, AppAction> = ( | |||
| export const hostListReducer: ImmutableReducer<HostListState, AppAction> = ( | |||
There was a problem hiding this comment.
another case where its probably not necessary, but it is helpful for debugging and stuff
x-pack/plugins/endpoint/public/applications/endpoint/store/immutable_combine_reducers.ts
Show resolved
Hide resolved
| return newState; | ||
| return { | ||
| ...newState, | ||
| isLoading: true, |
There was a problem hiding this comment.
newState isn't easy to mutate because its kinda a mix between immutable and not. Its created by spreading an immutable value, so all of its nonprimivite keys are immutable. We could come up w/ a type for this if we need it
| const newPolicy: PolicyConfig = { ...fullPolicy(state) }; | ||
|
|
||
| /** | ||
| * This is directly changing redux state because `policyItem.inputs` was copied over and not cloned. |
There was a problem hiding this comment.
not 100% sure that this is mutating redux state, but im pretty sure its not safe
There was a problem hiding this comment.
yeah, you are right here. Missed that. I'm thinking that we should just start to use lodash cloneDeep especially when working with these deep object structures.
| /** | ||
| * this is not safe because `action.payload.policyConfig` may have excess keys | ||
| */ | ||
| // @ts-ignore |
There was a problem hiding this comment.
suggestion: store the action.payload on state plain and simple
return {
userModifiedPolicyConfig: action.payload
}
Then in the middleware when you need to make a request, create a new object that is a combination of the policy (as it was received from the server) and the user modified parts.
There was a problem hiding this comment.
My initial goal was actually the opposite - ensure the state.policyItem is alway a full representation of the datasource/policy and create selectors that return back a sub-section of that for use in the UI.
I'm good with changing the approach if that makes things easier to work with from a TS standpoint.
There was a problem hiding this comment.
gotcha. My suggestion wasn't really to get around any TS issues. I try to model redux state as a normalized representation of the application state. In this case, the form dispatches a series of actions, each including the entire state of the form. That's why I recommended making the state be simply the payload of the action. This also keeps writing to the redux store cheap, which is not a primary goal, but doesn't hurt.
I would say that merging the form state with other metadata to make a request to another plugin would be the concern of the data layer, that's why I recommend doing it in the middleware.
Anyway, just my 2c
| /** | ||
| * this is not safe because `action.payload.policyConfig` may have excess keys | ||
| */ | ||
| // @ts-ignore |
There was a problem hiding this comment.
My initial goal was actually the opposite - ensure the state.policyItem is alway a full representation of the datasource/policy and create selectors that return back a sub-section of that for use in the UI.
I'm good with changing the approach if that makes things easier to work with from a TS standpoint.
| const newPolicy: PolicyConfig = { ...fullPolicy(state) }; | ||
|
|
||
| /** | ||
| * This is directly changing redux state because `policyItem.inputs` was copied over and not cloned. |
There was a problem hiding this comment.
yeah, you are right here. Missed that. I'm thinking that we should just start to use lodash cloneDeep especially when working with these deep object structures.
oatkiller
force-pushed
the
redux-types
branch
2 times, most recently
from
fe18fae
to
43cf87a
Compare
| @@ -6,11 +6,8 @@ | |||
|
|
|||
| import uuid from 'uuid'; | |||
| import seedrandom from 'seedrandom'; | |||
| import { AlertEvent, EndpointEvent, HostMetadata, OSFields, HostFields } from './types'; | |||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | |||
| */ | ||
| export const generatePolicy = (): PolicyConfig => { | ||
| export const factory = (): PolicyConfig => { |
There was a problem hiding this comment.
removing generate from name, as this is a pure function and 'generate' is being used by the 'data generator' scripts and related code
| /** | ||
| * Endpoint Policy configuration | ||
| */ | ||
| export interface PolicyConfig { |
There was a problem hiding this comment.
Used by code in common, so moved to common
| * Model for the `IIndexPattern` interface exported by the `data` plugin. | ||
| */ | ||
| export function clone(value: IIndexPattern | Immutable<IIndexPattern>): IIndexPattern { | ||
| return all([value]) as IIndexPattern; |
There was a problem hiding this comment.
@paul-tavares i'm on the same page as you re: using libraries to clone and/or support other immutable patterns. I'm using all from deepmerge here. The one in lodash had too much code for me to read and understand
| policyDetails, | ||
| policyData => { | ||
| return policyData?.inputs[0]?.config?.policy?.value ?? generatePolicy(); | ||
| return policyData?.inputs[0]?.config?.policy?.value ?? defaultFullPolicy; |
There was a problem hiding this comment.
any time we return a default policy, return the same (immutable) object reference
| /** | ||
| * A description for the component. | ||
| */ | ||
| description: string; |
There was a problem hiding this comment.
description was being passed an empty string.
| /** | ||
| * Types of supported operating systems. | ||
| */ | ||
| supportedOss: React.ReactNode; |
There was a problem hiding this comment.
this was an array of translated strings. it was being concat'd with , but that should be done via i18n. taking a react node instead.
| /** | ||
| * The `data-test-subj` attribute to append to a certain child element. | ||
| */ | ||
| dataTestSubj: string; |
There was a problem hiding this comment.
renaming from id because its just used for data-test-subj and id already has other meanings
| @@ -123,7 +123,7 @@ export const MalwareProtections = React.memo(() => { | |||
| [dispatch, policyDetailsConfig] | |||
| ); | |||
|
|
|||
| const RadioButtons = () => { | |||
| const radioButtons = useMemo(() => { | |||
There was a problem hiding this comment.
capital functions in react are conventionally reserved for components
| @@ -44,8 +44,8 @@ const PolicyLink: React.FC<{ name: string; route: string }> = ({ name, route }) | |||
| ); | |||
| }; | |||
|
|
|||
| const renderPolicyNameLink = (value: string, _item: PolicyData) => { | |||
| return <PolicyLink name={value} route={`/policy/${_item.id}`} />; | |||
| const renderPolicyNameLink = (value: string, item: Immutable<PolicyData>) => { | |||
There was a problem hiding this comment.
_ prefix in typescript bindings is used to tell tsserver to exclude it from unused-bindings checks. this is used
| @@ -160,7 +160,7 @@ export const PolicyList = React.memo(() => { | |||
| } | |||
| > | |||
| <EuiBasicTable | |||
| items={policyItems} | |||
| items={useMemo(() => [...policyItems], [policyItems])} | |||
There was a problem hiding this comment.
EuiBasicTable doesn't accept readonly arrays. We could probably get that changed in EUI, as it probably doesn't actually mutate items
oatkiller
added
release_note:skip
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
| export interface PolicyConfig { | ||
| windows: { | ||
| events: { | ||
| dll_and_driver_load: boolean; |
There was a problem hiding this comment.
were we thinking about making these events keys part of another enum?
There was a problem hiding this comment.
Not sure what you mean. This was moved to common because it was used by (or was related to a type used by) the generator code (which is in common.)
| export type HostAction = | ||
| | ServerReturnedHostList | ||
| | ServerReturnedHostDetails | ||
| | ServerFailedToReturnHostDetails | ||
| | UserPaginatedHostList | ||
| | FakeActionWithNoPayload; | ||
| | UserPaginatedHostList; |
There was a problem hiding this comment.
what helped us get around this issue?
| description={i18n.translate('xpack.endpoint.policy.details.eventCollectionLabel', { | ||
| defaultMessage: 'Event Collection', | ||
| })} | ||
| supportedOss={i18n.translate('xpack.endpoint.policy.details.linux', { |
There was a problem hiding this comment.
why did you get rid of useMemo here again?
There was a problem hiding this comment.
supportedOss was taking an array of translated strings and then concat'ing them with ', '. Since we do i18n, the translator needs to be able to specify that text. supportedOss now takes a React node. Since i18n.translate is returning a string, there is no need to memoize.
## Summary Changes some types around so modifying redux state or actions will cause type errors * the state and action types aren't using `Immutable` directly. This means that you can instantiate them and mutate them * the `state` and `action` params received by all reducers will be automatically cast to `Immutable` * reducers can return either `Immutable` or regular versions of `state` * some code may be mutating redux state directly, this is being ignored (at least for now)
## Summary Changes some types around so modifying redux state or actions will cause type errors * the state and action types aren't using `Immutable` directly. This means that you can instantiate them and mutate them * the `state` and `action` params received by all reducers will be automatically cast to `Immutable` * reducers can return either `Immutable` or regular versions of `state` * some code may be mutating redux state directly, this is being ignored (at least for now)
## Summary Changes some types around so modifying redux state or actions will cause type errors * the state and action types aren't using `Immutable` directly. This means that you can instantiate them and mutate them * the `state` and `action` params received by all reducers will be automatically cast to `Immutable` * reducers can return either `Immutable` or regular versions of `state` * some code may be mutating redux state directly, this is being ignored (at least for now)
Summary
Changes some types around so modifying redux state or actions will cause type errors
Immutabledirectly. This means that you can instantiate them and mutate themstateandactionparams received by all reducers will be automatically cast toImmutableImmutableor regular versions ofstateTODO
stateandactionreceived in middleware should beImmutable(next PR, since this got big)Checklist
Delete any items that are not applicable to this PR.
For maintainers