From 4a322c8e1a17fa58cefd7dc2e951b3f3cf25eca2 Mon Sep 17 00:00:00 2001 From: Marco polo Date: Fri, 2 Feb 2024 13:58:49 -0500 Subject: [PATCH] Rename "initialState" to "state" in useStaticSetFilter and useTimeRangeFilter (#19561) ## Summary & Motivation Renaming initialState to state since it better represents what the prop is, though even this is still a bit misleading. Let me explain: These filters were written in such a way to support being uncontrolled and controlled at the same time while allowing switching between uncontrolled and controlled. Basically the filter object returned by this hook has a property `state` which behaves like an uncontrolled component. Meaning whenever the user interacts with the UI, the UI will update accordingly and ignore the state prop you passed in, that means every time the user interacts the hook goes from controlled -> uncontrolled automatically. The one case where this could trip a developer up is in the case where the developer tries to ignore a state update by no-oping in `onStateChanged`(meaning they don't update their copy of the filter state). In that case since the state passed into the hook has not changed, the component will keep its uncontrolled copy of the state (which has diverged). To properly implement a full controlled instance of the hook you would need to create a copy of the old state (a new reference) and pass that in. Why allow both controlled and uncontrolled: Developer ergonomics, In the case where you don't need any fancy translation via the `onStateChanged` callback then you can just rely on the uncontrolled mode. [Here is an example of where we do that](https://github.com/dagster-io/dagster/blob/beff9f6147c68d10bdd49499eb28a64b7ba5d185/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useInstigationStatusFilter.tsx#L5) https://github.com/dagster-io/dagster/blob/beff9f6147c68d10bdd49499eb28a64b7ba5d185/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewSchedulesRoot.tsx#L83 ## How I Tested These Changes Tested filtering on a few pages to make sure it still works, also jest tests. --- .../asset-graph/AssetGraphExplorerFilters.tsx | 4 +-- .../ui-core/src/runs/RunsFilterInput.tsx | 14 ++++---- .../__tests__/useStaticSetFilter.test.tsx | 4 +-- .../__tests__/useTimeRangeFilter.test.tsx | 8 ++--- .../src/ui/Filters/useCodeLocationFilter.tsx | 2 +- .../src/ui/Filters/useStaticSetFilter.tsx | 28 +++++++++------- .../src/ui/Filters/useTimeRangeFilter.tsx | 33 ++++++++++--------- 7 files changed, 50 insertions(+), 43 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetGraphExplorerFilters.tsx b/js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetGraphExplorerFilters.tsx index f3ae6e1ac5e4a..abba6eb924551 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetGraphExplorerFilters.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetGraphExplorerFilters.tsx @@ -90,7 +90,7 @@ export function useAssetGraphExplorerFilters({ ' - ' + buildRepoPathForHuman(group.repositoryName, group.repositoryLocationName), - initialState: useMemo(() => new Set(visibleAssetGroups ?? []), [visibleAssetGroups]), + state: useMemo(() => new Set(visibleAssetGroups ?? []), [visibleAssetGroups]), onStateChanged: (values) => { if (setGroupFilters) { setGroupFilters(Array.from(values)); @@ -125,7 +125,7 @@ export function useAssetGraphExplorerFilters({ ), getStringValue: (value) => value, - initialState: computeKindTags ?? emptyArray, + state: computeKindTags ?? emptyArray, onStateChanged: (values) => { setComputeKindTags?.(Array.from(values)); }, diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunsFilterInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunsFilterInput.tsx index 50d62e9dbad59..6fda003a45af5 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunsFilterInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunsFilterInput.tsx @@ -298,7 +298,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte ), getStringValue: (x) => x, - initialState: useMemo( + state: useMemo( () => new Set(tokens.filter((x) => x.token === 'job').map((x) => x.value)), [tokens], ), @@ -319,7 +319,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte allValues: StatusFilterValues, renderLabel: ({value}) => {capitalizeFirstLetter(value)}, getStringValue: (x) => capitalizeFirstLetter(x), - initialState: useMemo( + state: useMemo( () => new Set(tokens.filter((x) => x.token === 'status').map((x) => x.value)), [tokens], ), @@ -346,7 +346,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte ), getStringValue: (x) => x, - initialState: useMemo( + state: useMemo( () => new Set(tokens.filter((x) => x.token === 'job').map((x) => x.value)), [tokens], ), @@ -366,7 +366,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte icon: 'backfill', allValues: backfillValues, allowMultipleSelections: false, - initialState: useMemo(() => { + state: useMemo(() => { return new Set( tokens .filter( @@ -403,7 +403,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte icon: 'partition', allValues: partitionValues, allowMultipleSelections: false, - initialState: useMemo(() => { + state: useMemo(() => { return new Set( tokens .filter( @@ -466,7 +466,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte } return x.value!; }, - initialState: useMemo(() => { + state: useMemo(() => { return new Set( tokens .filter( @@ -495,7 +495,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte const createdDateFilter = useTimeRangeFilter({ name: 'Created date', icon: 'date', - initialState: useMemo(() => { + state: useMemo(() => { const before = tokens.find((token) => token.token === 'created_date_before'); const after = tokens.find((token) => token.token === 'created_date_after'); return [ diff --git a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useStaticSetFilter.test.tsx b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useStaticSetFilter.test.tsx index 7c393749f28ef..a0d03dc3b1b65 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useStaticSetFilter.test.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useStaticSetFilter.test.tsx @@ -23,7 +23,7 @@ describe('useStaticSetFilter', () => { {value} ), getStringValue: (value: string) => value, - initialState: ['banana'], + state: ['banana'], }; it('creates filter object with the correct properties', () => { @@ -114,7 +114,7 @@ describe('useStaticSetFilter', () => { select(filter, 'banana'); expect(filter.result.current.state).toEqual(new Set(['apple'])); - props.initialState = ['cherry']; + props.state = ['cherry']; filter.rerender(); diff --git a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useTimeRangeFilter.test.tsx b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useTimeRangeFilter.test.tsx index 20eaa8de19302..fe9f2b1582cd0 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useTimeRangeFilter.test.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/__tests__/useTimeRangeFilter.test.tsx @@ -25,7 +25,7 @@ jest.mock('react-dates', () => { const mockFilterProps = { name: 'Test Filter', icon: 'date' as IconName, - initialState: [null, null] as TimeRangeState, + state: [null, null] as TimeRangeState, }; describe('useTimeRangeFilter', () => { @@ -35,7 +35,7 @@ describe('useTimeRangeFilter', () => { expect(filter.name).toBe(mockFilterProps.name); expect(filter.icon).toBe(mockFilterProps.icon); - expect(filter.state).toEqual(mockFilterProps.initialState); + expect(filter.state).toEqual(mockFilterProps.state); }); it('should reset filter state', () => { @@ -47,14 +47,14 @@ describe('useTimeRangeFilter', () => { }); filter = result.current; - expect(filter.state).not.toEqual(mockFilterProps.initialState); + expect(filter.state).not.toEqual(mockFilterProps.state); act(() => { filter.setState([null, null]); }); filter = result.current; - expect(filter.state).toEqual(mockFilterProps.initialState); + expect(filter.state).toEqual(mockFilterProps.state); }); it('should handle pre-defined time ranges', () => { diff --git a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useCodeLocationFilter.tsx b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useCodeLocationFilter.tsx index e16817cf7cbbc..d3321cee2953f 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useCodeLocationFilter.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useCodeLocationFilter.tsx @@ -27,7 +27,7 @@ export const useCodeLocationFilter = () => { return useStaticSetFilter({ name: 'Code location', icon: 'folder', - initialState: visibleRepoAddresses, + state: visibleRepoAddresses, allValues: allRepoAddresses.map((repoAddress) => { return {value: repoAddress, match: [repoAddressAsHumanString(repoAddress)]}; }), diff --git a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useStaticSetFilter.tsx b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useStaticSetFilter.tsx index 68aa219174a28..56cf4fb35096d 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useStaticSetFilter.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useStaticSetFilter.tsx @@ -18,7 +18,12 @@ type Args = { getStringValue: (value: TValue) => string; getTooltipText?: (value: TValue) => string; allValues: SetFilterValue[]; - initialState?: Set | TValue[]; + + // This hook is NOT a "controlled component". Changing state only updates the component's current state. + // To make this fully controlled you need to implement `onStateChanged` and maintain your own copy of the state. + // The one tricky footgun is if you want to ignore (ie. cancel) a state change then you need to make a new reference + // to the old state and pass that in. + state?: Set | TValue[]; onStateChanged?: (state: Set) => void; allowMultipleSelections?: boolean; matchType?: 'any-of' | 'all-of'; @@ -38,7 +43,7 @@ export function useStaticSetFilter({ allValues: _unsortedValues, renderLabel, renderActiveStateLabel, - initialState, + state, getStringValue, getTooltipText, onStateChanged, @@ -57,23 +62,24 @@ export function useStaticSetFilter({ return _unsortedValues; }, [StaticFilterSorter, name, _unsortedValues]); - const [state, setState] = useState(() => new Set(initialState || [])); + // This filter can be used as both a controlled and an uncontrolled component necessitating an innerState for the uncontrolled case. + const [innerState, setState] = useState(() => new Set(state || [])); useEffect(() => { - onStateChanged?.(state); + onStateChanged?.(innerState); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [state]); + }, [innerState]); useEffect(() => { - setState(initialState ? new Set(initialState) : new Set()); - }, [initialState]); + setState(state ? new Set(state) : new Set()); + }, [state]); const filterObj: StaticSetFilter = useMemo( () => ({ name, icon, - state, - isActive: state.size > 0, + state: innerState, + isActive: innerState.size > 0, getResults: (query) => { if (query === '') { return allValues.map(({value}, index) => ({ @@ -127,7 +133,7 @@ export function useStaticSetFilter({ activeJSX: ( ({ [ name, icon, - state, + innerState, getStringValue, renderActiveStateLabel, renderLabel, diff --git a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useTimeRangeFilter.tsx b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useTimeRangeFilter.tsx index 673054e5cdc20..95a6746a54921 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useTimeRangeFilter.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/ui/Filters/useTimeRangeFilter.tsx @@ -69,28 +69,29 @@ type TimeRangeKey = keyof ReturnType['timeRanges']; type Args = { name: string; icon: IconName; - initialState?: TimeRangeState; + + // This hook is NOT a "controlled component". Changing state only updates the component's current state. + // To make this fully controlled you need to implement `onStateChanged` and maintain your own copy of the state. + // The one tricky footgun is if you want to ignore (ie. cancel) a state change then you need to make a new reference + // to the old state and pass that in. + state?: TimeRangeState; onStateChanged?: (state: TimeRangeState) => void; }; -export function useTimeRangeFilter({ - name, - icon, - initialState, - onStateChanged, -}: Args): TimeRangeFilter { +export function useTimeRangeFilter({name, icon, state, onStateChanged}: Args): TimeRangeFilter { const { timezone: [_timezone], } = useContext(TimeContext); const timezone = _timezone === 'Automatic' ? browserTimezone() : _timezone; - const [state, setState] = useState(initialState || [null, null]); + const [innerState, setState] = useState(state || [null, null]); + useEffect(() => { - onStateChanged?.(state); + onStateChanged?.(innerState); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [state[0], state[1]]); + }, [innerState[0], innerState[1]]); useEffect(() => { - setState(initialState || [null, null]); - }, [initialState]); + setState(state || [null, null]); + }, [state]); const {timeRanges, timeRangesArray} = useMemo( () => calculateTimeRanges(timezone), @@ -110,9 +111,9 @@ export function useTimeRangeFilter({ () => ({ name, icon, - state, + state: innerState, setState, - isActive: state[0] !== null || state[1] !== null, + isActive: innerState[0] !== null || innerState[1] !== null, getResults: ( query: string, ): { @@ -153,14 +154,14 @@ export function useTimeRangeFilter({ activeJSX: ( ), }), // eslint-disable-next-line react-hooks/exhaustive-deps - [name, icon, state, timeRanges, timezone, timeRangesArray], + [name, icon, innerState, timeRanges, timezone, timeRangesArray], ); const filterObjRef = useUpdatingRef(filterObj); return filterObj;