When a Hook Knows Too Much
In this article, we will try and refactor some real code from our codebase. The code has been simplified for the sake of the demo.
export interface UseCreateDraftHandlerProps {
onDraftDialogOpen: () => void;
goToSettingPricingGroupEditPage: (id: string) => void;
onErrorDialogOpen: () => void;
onSelectedDraftIdChange: (id: string) => void;
onDraftLoadingChange: (isLoading: boolean) => void;
}
export function useCreateDraftHandler(
{
onDraftDialogOpen,
goToSettingPricingGroupEditPage,
onErrorDialogOpen,
onSelectedDraftIdChange,
onDraftLoadingChange,
}: UseCreateDraftHandlerProps,
) {
const dispatch = useDispatch1();
return (model: PricingGroup) => {
onSelectedDraftIdChange('');
onDraftLoadingChange(true);
dispatch(getPricingGroupDraft({
request: { id: model.id },
correlation_id: nanoid(),
}))
.unwrap()
.then((res) => {
onSelectedDraftIdChange(res.data.id);
if (res.data.type === 'old') {
onDraftDialogOpen();
}
else {
return goToSettingPricingGroupEditPage(res.data.id);
}
})
.catch(() => onErrorDialogOpen())
.finally(() => onDraftLoadingChange(false));
};
}
Here is a snippet from our codebase. Without context, it’s hard to see why this function exists. Why are we setting a draft id to empty? Why are we opening dialog at times but going to page another time?
In YoPrint, anytime a user wants to edit a Pricing Group, we will create a copy (or draft). Then the user can make changes to the draft without affecting live pricing. Once satisfied, the user will publish the draft finalizing the changes.
The code above first fetches a draft. The backend will create a new draft if one doesn’t exist or return an existing draft to the frontend. If it’s a new draft, then we will take them straight to the edit page. Otherwise, we will ask the user if they want to continue editing the old draft or would like to create a new one. We added this so that the user is not editing a draft that is too old or wants to start fresh.
Voyeuristic Child Component
This component knows too much about the caller. For example, this code assumes the caller will open a confirmation dialog for old draft. What if the UI changed so that we use a popover instead of a dialog? This code also assumes editing a Pricing Group Draft will take place in a page but what if the UI changes that to a dialog?
What is the purpose of this code?
The purpose of this hook is to provide a callback that calls the getPricingGroupDraft endpoint and advice the caller on the next course of action. The caller will simply wire the UI based on the decision.
First Sweep
It needs to know which Pricing Group it needs to fetch the draft for. This is ALL the hook needs to function.
export interface UseCreateDraftHandlerProps {
onFetchStarted: () => void;
onOldDraftDetected: (draft: PricingGroupDraft) => void;
onNewDraftDetected: (draft: PricingGroupDraft) => void;
onFetchFailed: () => void;
onFetchCompleted: () => void;
}
export function useCreateDraftHandler(props: UseCreateDraftHandlerProps) {
const dispatch = useDispatch();
return (model: PricingGroup) => {
props.onFetchStarted();
dispatch(getPricingGroupDraft({
request: { id: model.id },
correlation_id: nanoid(),
}))
.unwrap()
.then((res) => {
if (res.data.type === 'old') {
props.onOldDraftDetected(res.data);
}
else {
props.onNewDraftDetected(res.data);
}
})
.catch(() => props.onFetchFailed())
.finally(() => props.onFetchCompleted());
};
}
The rewrite makes it easy to guess the intent of the component. It also doesn’t leak any of the caller-related states into itself.
Second Sweep
If you are following SRP strictly, you can leave it as above. I feel like we could change the definition of this function.
Based on YoPrint, we know that users can only edit one Pricing Group at a time. If that’s the case, we can move even more logic into the hook.
export interface UseCreateDraftHandlerProps {
onDraftSelected: (draft: PricingGroupDraft) => void;
}
export function useCreateDraftHandler(props: UseCreateDraftHandlerProps) {
const dispatch = useDispatch();
const [isFetching, setIsFetching] = useState<boolean>(false);
const [isFetchFailed, setIsFetchFailed] = useState<boolean>(false);
const [isShowConfirmation, setIsShowConfirmation] = useState<boolean>(false);
const [draft, setDraft] = useState<PricingGroupDraft | null>(null);
const [pricingGroup, setPricingGroup] = useState<PricingGroup | null>(null);
const onCancel = () => {
setIsShowConfirmation(false);
}
const onResumeDraft = () => {
setIsShowConfirmation(false);
if (draft) {
onDraftSelected(draft);
}
}
const onNewDraft = () => {
setIsShowConfirmation(false);
onFetchDraft(pricingGroup, true);
}
const onFetchDraft = (model: PricingGroup, forceNewDraft?: boolean) => {
setIsFetching(true);
setIsFetchFailed(false);
setPricingGroup(model);
dispatch(getPricingGroupDraft({
id: model.id,
forceNewDraft,
}))
.unwrap()
.then((res) => {
if (res.data.type === 'old') {
setDraft(res.data)
setIsShowConfirmation(true);
}
else {
onDraftSelected(res.data);
}
})
.catch(() => setIsFetchFailed(true))
.finally(() => setIsFetching(false));
};
return {
draft,
isFetching,
isFetchFailed,
isShowConfirmation,
onCancel,
onResumeDraft,
onFetchDraft,
}
}
Depending on how you structure your UI, this approach might work better. Here the caller can wire the onFetchDraft to the Edit button to kick-start the interaction.
The loading error state is now local to the webhooks. We also return a boolean to let the caller know when to show the confirmation UI and provide the appropriate callbacks.
Now this component handles the entire decision-making and advises the caller of appropriate actions without needing to know about the parent components.
Hope this helps!