trojanowski / react-apollo-hooks

Use Apollo Client as React hooks
MIT License
2.42k stars 110 forks source link

return memoized value #33

Open sijad opened 5 years ago

sijad commented 5 years ago

currently returned value both of useMutation and useQuery calculated every time they called, I'm not really familiar with apollo logics, but I guess this can memoized?, for instance useMutation should be similar like this:

export function useMutation(mutation, baseOptions) {
  const previousOptions = useRef();
  const client = useApolloClient();
  const result = isEqual(previousOptions.current, baseOptions);
  return useMemo(
    () => {
      previousOptions.current = baseOptions;
      return localOptions => client.mutate({
        mutation,
        ...baseOptions,
        ...localOptions,
      });
    },
    [
      mutation,
      result ? previousOptions.current : baseOptions,
    ],
  );
}
trojanowski commented 5 years ago

@sijad Have you tried to profile it if the version with memoization is really faster than the current one?

sijad commented 5 years ago

no, I did not, but I guess with memorized version you can actually prevent re-rendering e.g:

const mutate = useMutation(MUTATION)
const memoizedMutation = useCallback(() => {
  mutate({
    variables,
  });
}, [mutate]);

return (<button onClick={memoizedMutation}>click on me</button>);

with current implimention mutate always have a new value therefore it might re-render more...

avocadowastaken commented 5 years ago

Checking for mutation itself is not enough, we have to check for changes of options parameters, and this will create unnecessary memoization most of the times.

Anyways, React team does not suggest to pass individual callbacks in props and there is (not ideal) solution for this. I hope they will come up with better idea.

But if you don't like it, you still can use your own hook for memoization , e.g:

function useMemoMutation(mutation, options, inputs) {
   return useCallback(useMutation(mutation, option), inputs);
}

function Foo({ bar, baz }) {
  const mutate = useMemoMutation(Mutate, {
    variables: { bar, baz }
  }, [bar, baz]) // Update only on `bar` or `baz` change
}
avocadowastaken commented 5 years ago

We also can useRef to imitate this in class components:

export function useMutation<TData, TVariables = OperationVariables>(
  mutation: DocumentNode,
  baseOptions?: MutationHookOptions<TData, TVariables>
): MutationFn<TData, TVariables> {
  const client = useApolloClient();

  // Use refs to access variable values inside callback.
  const mutationRef = useRef(mutation);
  const baseOptionsRef = useRef(baseOptions);

  // Update references on each render.
  mutationRef.current = mutation;
  baseOptionsRef.current = baseOptions;

  // Memoize callback.
  const mutationFnRef = useRef<MutationFn<TData, TVariables>>(options =>
    client.mutate({
      mutation: mutationRef.current,
      ...baseOptionsRef.current,
      ...options,
    })
  );

  return mutationFnRef.current;
}
rovansteen commented 5 years ago

A downside of not memoizing the value of useMutation or useQuery is that they break the memoization of hooks that are built on top of them if you follow the rules (and eslint plugin) for hooks strictly.

sijad commented 5 years ago

I just leave @gaearon's comment here.

in my test objToKey seems slower that deep equal.

prztrz commented 4 years ago

should not this be closed by https://github.com/apollographql/react-apollo/issues/3260 ?

fbartho commented 4 years ago

@prztrz this library doesn’t use React-apollo’s useMutation (this preceded that!)

If you switch to @apollo/hooks or whatever that package name is, then that fix will be relevant. Cheers!