Skip to content

Commit

Permalink
Rename "initialState" to "state" in useStaticSetFilter and useTimeRan…
Browse files Browse the repository at this point in the history
…geFilter (#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.
  • Loading branch information
salazarm authored Feb 2, 2024
1 parent 143d555 commit 4a322c8
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -125,7 +125,7 @@ export function useAssetGraphExplorerFilters({
</Box>
),
getStringValue: (value) => value,
initialState: computeKindTags ?? emptyArray,
state: computeKindTags ?? emptyArray,
onStateChanged: (values) => {
setComputeKindTags?.(Array.from(values));
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte
</Box>
),
getStringValue: (x) => x,
initialState: useMemo(
state: useMemo(
() => new Set(tokens.filter((x) => x.token === 'job').map((x) => x.value)),
[tokens],
),
Expand All @@ -319,7 +319,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte
allValues: StatusFilterValues,
renderLabel: ({value}) => <span>{capitalizeFirstLetter(value)}</span>,
getStringValue: (x) => capitalizeFirstLetter(x),
initialState: useMemo(
state: useMemo(
() => new Set(tokens.filter((x) => x.token === 'status').map((x) => x.value)),
[tokens],
),
Expand All @@ -346,7 +346,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte
</Box>
),
getStringValue: (x) => x,
initialState: useMemo(
state: useMemo(
() => new Set(tokens.filter((x) => x.token === 'job').map((x) => x.value)),
[tokens],
),
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -466,7 +466,7 @@ export const useRunsFilterInput = ({tokens, onChange, enabledFilters}: RunsFilte
}
return x.value!;
},
initialState: useMemo(() => {
state: useMemo(() => {
return new Set(
tokens
.filter(
Expand Down Expand Up @@ -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 [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('useStaticSetFilter', () => {
<span className={isActive ? 'active' : 'inactive'}>{value}</span>
),
getStringValue: (value: string) => value,
initialState: ['banana'],
state: ['banana'],
};

it('creates filter object with the correct properties', () => {
Expand Down Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const useCodeLocationFilter = () => {
return useStaticSetFilter<RepoAddress>({
name: 'Code location',
icon: 'folder',
initialState: visibleRepoAddresses,
state: visibleRepoAddresses,
allValues: allRepoAddresses.map((repoAddress) => {
return {value: repoAddress, match: [repoAddressAsHumanString(repoAddress)]};
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ type Args<TValue> = {
getStringValue: (value: TValue) => string;
getTooltipText?: (value: TValue) => string;
allValues: SetFilterValue<TValue>[];
initialState?: Set<TValue> | 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> | TValue[];
onStateChanged?: (state: Set<TValue>) => void;
allowMultipleSelections?: boolean;
matchType?: 'any-of' | 'all-of';
Expand All @@ -38,7 +43,7 @@ export function useStaticSetFilter<TValue>({
allValues: _unsortedValues,
renderLabel,
renderActiveStateLabel,
initialState,
state,
getStringValue,
getTooltipText,
onStateChanged,
Expand All @@ -57,23 +62,24 @@ export function useStaticSetFilter<TValue>({
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<TValue> = useMemo(
() => ({
name,
icon,
state,
isActive: state.size > 0,
state: innerState,
isActive: innerState.size > 0,
getResults: (query) => {
if (query === '') {
return allValues.map(({value}, index) => ({
Expand Down Expand Up @@ -127,7 +133,7 @@ export function useStaticSetFilter<TValue>({
activeJSX: (
<SetFilterActiveState
name={name}
state={state}
state={innerState}
getStringValue={getStringValue}
getTooltipText={getTooltipText}
renderLabel={renderActiveStateLabel || renderLabel}
Expand All @@ -145,7 +151,7 @@ export function useStaticSetFilter<TValue>({
[
name,
icon,
state,
innerState,
getStringValue,
renderActiveStateLabel,
renderLabel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,28 +69,29 @@ type TimeRangeKey = keyof ReturnType<typeof calculateTimeRanges>['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<TimeRangeState>(initialState || [null, null]);
const [innerState, setState] = useState<TimeRangeState>(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),
Expand All @@ -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,
): {
Expand Down Expand Up @@ -153,14 +154,14 @@ export function useTimeRangeFilter({
activeJSX: (
<ActiveFilterState
timeRanges={timeRanges}
state={state}
state={innerState}
timezone={timezone}
remove={onReset}
/>
),
}),
// 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;
Expand Down

1 comment on commit 4a322c8

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-dlex3zmuk-elementl.vercel.app

Built with commit 4a322c8.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.