Skip to content

Commit

Permalink
Add "isEnabled" prop to useAssetcatalogFiltering to allow calling h…
Browse files Browse the repository at this point in the history
…ook hook while disabling all of its side effects. (#23686)

## Summary & Motivation

In cloud we want to conditionally call this hook, or an elastic search
backed version of the hook, but we can't do that because it would break
the rules of hooks. Unfortunately the place where we want to call it
conditionally is inside of another hook used by multiple different
components and even other hooks, so going to the parent component and
making them conditionally pass 1 version of the hook or another would
not be very elegant since there would be a lot of threading of hooks
around from places which really shouldn't know anything about this hook
or its implementation. So... instead of that what we're doing is calling
both hooks, but only returning the results of the hook we're using. The
problem is that this hook has global side effects (by way of
CatalogViewProvider integration and useQueryPersistedState) so I'm
adding an `isEnabled` prop so that we disable all of these side effects
and avoid bugs.

## How I Tested These Changes

Tested with https://github.com/dagster-io/internal/pull/11040/files
  • Loading branch information
salazarm authored Aug 19, 2024
1 parent fc26db2 commit 0827970
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,12 @@ export function useAssetCatalogFiltering<
assets = EMPTY_ARRAY,
includeRepos = true,
loading = false,
isEnabled = true,
}: {
assets: T[] | undefined;
includeRepos?: boolean;
loading?: boolean;
isEnabled?: boolean;
}) {
const {
filters,
Expand All @@ -53,7 +55,7 @@ export function useAssetCatalogFiltering<
setCodeLocations,
setStorageKindTags,
setSelectAllFilters,
} = useAssetDefinitionFilterState();
} = useAssetDefinitionFilterState({isEnabled});

const allAssetGroupOptions = useAssetGroupSelectorsForAssets(assets);
const allComputeKindTags = useAssetKindTagsForAssets(assets);
Expand Down Expand Up @@ -161,7 +163,7 @@ export function useAssetCatalogFiltering<
* This effect handles syncing the `selectAllFilters` query param state with the actual filtering state.
* eg: If all of the items are selected then we include that key, otherwise we remove it.
*/
if (loading) {
if (loading || !isEnabled) {
return;
}
if (!didWaitAfterLoading) {
Expand Down Expand Up @@ -211,6 +213,7 @@ export function useAssetCatalogFiltering<
nonStorageKindTags,
setSelectAllFilters,
storageKindTags,
isEnabled,
]);

const filtered = React.useMemo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,39 +43,42 @@ export type AssetFilterType = AssetFilterBaseType & {
selectAllFilters: Array<keyof AssetFilterBaseType>;
};

export const useAssetDefinitionFilterState = () => {
export const useAssetDefinitionFilterState = ({isEnabled = true}: {isEnabled?: boolean}) => {
const [filters, setFilters] = useQueryPersistedState<AssetFilterType>({
encode: ({
groups,
computeKindTags,
storageKindTags,
changedInBranch,
owners,
tags,
codeLocations,
selectAllFilters,
}) => ({
groups: groups?.length ? JSON.stringify(groups) : undefined,
computeKindTags: computeKindTags?.length ? JSON.stringify(computeKindTags) : undefined,
storageKindTags: storageKindTags?.length ? JSON.stringify(storageKindTags) : undefined,
changedInBranch: changedInBranch?.length ? JSON.stringify(changedInBranch) : undefined,
owners: owners?.length ? JSON.stringify(owners) : undefined,
tags: tags?.length ? JSON.stringify(tags) : undefined,
codeLocations: codeLocations?.length ? JSON.stringify(codeLocations) : undefined,
selectAllFilters: selectAllFilters?.length ? JSON.stringify(selectAllFilters) : undefined,
}),
encode: isEnabled
? ({
groups,
computeKindTags,
storageKindTags,
changedInBranch,
owners,
tags,
codeLocations,
selectAllFilters,
}) => ({
groups: groups?.length ? JSON.stringify(groups) : undefined,
computeKindTags: computeKindTags?.length ? JSON.stringify(computeKindTags) : undefined,
storageKindTags: storageKindTags?.length ? JSON.stringify(storageKindTags) : undefined,
changedInBranch: changedInBranch?.length ? JSON.stringify(changedInBranch) : undefined,
owners: owners?.length ? JSON.stringify(owners) : undefined,
tags: tags?.length ? JSON.stringify(tags) : undefined,
codeLocations: codeLocations?.length ? JSON.stringify(codeLocations) : undefined,
selectAllFilters: selectAllFilters?.length ? JSON.stringify(selectAllFilters) : undefined,
})
: () => ({}),
decode: (qs) => ({
groups: qs.groups ? JSON.parse(qs.groups) : [],
computeKindTags: qs.computeKindTags ? JSON.parse(qs.computeKindTags) : [],
storageKindTags: qs.storageKindTags ? JSON.parse(qs.storageKindTags) : [],
changedInBranch: qs.changedInBranch ? JSON.parse(qs.changedInBranch) : [],
owners: qs.owners ? JSON.parse(qs.owners) : [],
tags: qs.tags ? JSON.parse(qs.tags) : [],
codeLocations: qs.codeLocations
? JSON.parse(qs.codeLocations).map((repo: RepoAddress) =>
buildRepoAddress(repo.name, repo.location),
)
: [],
groups: qs.groups && isEnabled ? JSON.parse(qs.groups) : [],
computeKindTags: qs.computeKindTags && isEnabled ? JSON.parse(qs.computeKindTags) : [],
storageKindTags: qs.storageKindTags && isEnabled ? JSON.parse(qs.storageKindTags) : [],
changedInBranch: qs.changedInBranch && isEnabled ? JSON.parse(qs.changedInBranch) : [],
owners: qs.owners && isEnabled ? JSON.parse(qs.owners) : [],
tags: qs.tags && isEnabled ? JSON.parse(qs.tags) : [],
codeLocations:
qs.codeLocations && isEnabled
? JSON.parse(qs.codeLocations).map((repo: RepoAddress) =>
buildRepoAddress(repo.name, repo.location),
)
: [],
selectAllFilters: qs.selectAllFilters ? JSON.parse(qs.selectAllFilters) : [],
}),
});
Expand Down

1 comment on commit 0827970

@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-o5zvwi0be-elementl.vercel.app

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

Please sign in to comment.