Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate a stable options object post-normalization #75

Closed
wants to merge 2 commits into from
Closed

Generate a stable options object post-normalization #75

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58067cb
Memoize options generation
btraut Oct 22, 2021
7cedd19
Fixed useMemo bug
btraut Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
} from 'react';
Expand Down Expand Up @@ -238,9 +239,12 @@ const useAsyncInternal = <R = UnknownResult, Args extends any[] = UnknownArgs>(

const normalizedOptions = normalizeOptions<R>(options);

// Use memoization to produce a stable options object post-normalization.
const memoizedOptions = useMemo(() => normalizedOptions, Object.values(normalizedOptions));
Copy link
Owner

Choose a reason for hiding this comment

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

Great catch

Unfortunately it won't work if values are updated, which may be the case because some options are callbacks and users might provide inlined arrow functions

I suggest using the useGetter() hook above to solve this more reliably once for all

  const getLatestOptions = useGetter(options);

  const reset = useCallback(() => setValue(getLatestOptions().initialState(getLatestOptions())), [
    setValue,
    getLatestOptions,
  ]);

(also worth applying to other methods as well, we don't follow the rules of hooks very strictly here 😅 )

Copy link
Author

Choose a reason for hiding this comment

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

Right, this still isn't stable if anon callbacks are passed, but it at least handles the case of empty options being passed (a potentially common use case).

Thanks for linking to your issue on useGetter in the code. It was fascinating to see other lib/hook authors face these changes. I don't want to pull the argument over here, but my read on the responses is that the useGetter pattern is pretty dangerous to use. In my limited experience, I've gone to great lengths to keep callbacks stable and I've documented to users what they are responsible for memoizing. It's an uphill battle and we need help from React core to make it easier, but I feel it's the safest solution in userland.

I don't feel comfortable updating to useGetter. If you'd like this PR as is, go ahead and merge it. Otherwise, I recommend closing it out or commandeering.


const [currentParams, setCurrentParams] = useState<Args | null>(null);

const AsyncState = useAsyncState<R>(normalizedOptions);
const AsyncState = useAsyncState<R>(memoizedOptions);

const isMounted = useIsMounted();
const CurrentPromise = useCurrentPromise<R>();
Expand All @@ -263,15 +267,15 @@ const useAsyncInternal = <R = UnknownResult, Args extends any[] = UnknownArgs>(
if (shouldHandlePromise(promise)) {
AsyncState.setResult(result);
}
normalizedOptions.onSuccess(result, {
memoizedOptions.onSuccess(result, {
isCurrent: () => CurrentPromise.is(promise),
});
},
error => {
if (shouldHandlePromise(promise)) {
AsyncState.setError(error);
}
normalizedOptions.onError(error, {
memoizedOptions.onError(error, {
isCurrent: () => CurrentPromise.is(promise),
});
}
Expand All @@ -291,8 +295,8 @@ const useAsyncInternal = <R = UnknownResult, Args extends any[] = UnknownArgs>(
const isMounting = !isMounted();
useEffect(() => {
const execute = () => getLatestExecuteAsyncOperation()(...params);
isMounting && normalizedOptions.executeOnMount && execute();
!isMounting && normalizedOptions.executeOnUpdate && execute();
isMounting && memoizedOptions.executeOnMount && execute();
!isMounting && memoizedOptions.executeOnUpdate && execute();
}, params);

return {
Expand Down