Skip to content
This repository has been archived by the owner on Nov 11, 2023. It is now read-only.

Commit

Permalink
Stabilize the identity of the mutate function
Browse files Browse the repository at this point in the history
Hopefully begins to address #186

Approach:
* Remove (mutable) references to state. Instead, hook into the
  (immutable) `setState` function in the mutate function body where
  state references are necessary
* Try very hard to be honest about what dependencies may change in the
  dependency array, but leave only immutable dependencies that depend only
  on props (almost totally successful)
  • Loading branch information
amacleay-cohere authored and fabien0102 committed Mar 22, 2021
1 parent 2793720 commit b75d2a6
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 22 deletions.
31 changes: 31 additions & 0 deletions src/useMutate.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,37 @@ describe("useMutate", () => {
});
});

describe("Mutate identity", () => {
it("should remain the same across calls", async () => {
nock("https://my-awesome-api.fake")
.post("/plop/one")
.reply(200, { id: 1 })
.persist();

const wrapper: React.FC = ({ children }) => (
<RestfulProvider base="https://my-awesome-api.fake">{children}</RestfulProvider>
);
const getPath = ({ id }: { id: string }) => `plop/${id}`;
const pathParams = { id: "one" };
const { result } = renderHook(
() =>
useMutate<{ id: number }, unknown, {}, {}, { id: string }>("POST", getPath, {
pathParams,
}),
{
wrapper,
},
);
const mutate0 = result.current.mutate;
const mutate1 = result.current.mutate;
await result.current.mutate({});
const mutate2 = result.current.mutate;

expect(mutate0).toEqual(mutate1);
expect(mutate0).toEqual(mutate2);
});
});

describe("Path Params", () => {
it("should resolve path parameters if specified", async () => {
nock("https://my-awesome-api.fake")
Expand Down
58 changes: 36 additions & 22 deletions src/useMutate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export function useMutate<
typeof arguments[0] === "object" ? arguments[0] : { ...arguments[2], path: arguments[1], verb: arguments[0] };

const context = useContext(Context);
const { verb, base = context.base, path, queryParams = {}, resolve, pathParams = {} } = props;
const { verb, base = context.base, path, queryParams = EMPTY_OBJECT, resolve, pathParams = EMPTY_OBJECT } = props;
const isDelete = verb === "DELETE";

const [state, setState] = useState<MutateState<TData, TError>>({
Expand All @@ -93,16 +93,15 @@ export function useMutate<
// Cancel the fetch on unmount
useEffect(() => () => abort(), [abort]);

const { pathInlineBodyEncode, queryParamStringifyOptions, requestOptions, localErrorOnly, onMutate } = props;
const mutate = useCallback<MutateMethod<TData, TRequestBody, TQueryParams, TPathParams>>(
async (body: TRequestBody, mutateRequestOptions?: MutateRequestOptions<TQueryParams, TPathParams>) => {
if (state.error || !state.loading) {
setState(prevState => ({ ...prevState, loading: true, error: null }));
}

if (state.loading) {
// Abort previous requests
abort();
}
setState(prevState => {
if (prevState.loading) {
abort();
}
return { ...prevState, loading: true, error: null };
});

const pathStr =
typeof path === "function" ? path(mutateRequestOptions?.pathParams || (pathParams as TPathParams)) : path;
Expand All @@ -123,9 +122,7 @@ export function useMutate<
} else if (typeof body === "object") {
options.body = JSON.stringify(body);
} else if (isDelete && body !== undefined) {
const possiblyEncodedBody = props.pathInlineBodyEncode
? props.pathInlineBodyEncode(String(body))
: String(body);
const possiblyEncodedBody = pathInlineBodyEncode ? pathInlineBodyEncode(String(body)) : String(body);

pathParts.push(possiblyEncodedBody);
} else {
Expand All @@ -139,14 +136,12 @@ export function useMutate<
pathParts.join("/"),
{ ...context.queryParams, ...queryParams, ...mutateRequestOptions?.queryParams },
{
queryParamOptions: { ...context.queryParamStringifyOptions, ...props.queryParamStringifyOptions },
queryParamOptions: { ...context.queryParamStringifyOptions, ...queryParamStringifyOptions },
},
);

const propsRequestOptions =
(typeof props.requestOptions === "function"
? await props.requestOptions(url, verb, body)
: props.requestOptions) || {};
(typeof requestOptions === "function" ? await requestOptions(url, verb, body) : requestOptions) || {};

const contextRequestOptions =
(typeof context.requestOptions === "function"
Expand Down Expand Up @@ -174,7 +169,7 @@ export function useMutate<
loading: false,
});

if (!props.localErrorOnly && context.onError) {
if (!localErrorOnly && context.onError) {
context.onError(error, () => mutate(body, mutateRequestOptions));
}

Expand Down Expand Up @@ -223,7 +218,7 @@ export function useMutate<
loading: false,
}));

if (!props.localErrorOnly && context.onError) {
if (!localErrorOnly && context.onError) {
context.onError(error, () => mutate(body), response);
}

Expand All @@ -232,14 +227,30 @@ export function useMutate<

setState(prevState => ({ ...prevState, loading: false }));

if (props.onMutate) {
props.onMutate(body, data);
if (onMutate) {
onMutate(body, data);
}

return data;
},
/* eslint-disable react-hooks/exhaustive-deps */
[context.base, context.requestOptions, context.resolve, state.error, state.loading, path, abort, getAbortSignal],
// eslint-disable-next-line react-hooks/exhaustive-deps
[
/* getAbortSignal - changes too much! */
path,
pathParams,
queryParams,
verb,
isDelete,
base,
context,
queryParamStringifyOptions,
requestOptions,
onMutate,
abort,
pathInlineBodyEncode,
localErrorOnly,
resolve,
],
);

return {
Expand All @@ -255,3 +266,6 @@ export function useMutate<
},
};
}

// Declaring this in order to have a thing with stable identity
const EMPTY_OBJECT = {};

0 comments on commit b75d2a6

Please sign in to comment.