Refetch when resolve prop changes

[carlesba]

Why

To provide new set of data when resolve prop changes.

Related Issue

#63

Issues

resolve is not matching its previous value when taking the defaultProps (i.e., when resolve prop is not defined). However, it works as expected when resolve is a defined (non-anonymous) function. That's the reason you can find resolve being passed in some tests.

I've tried to solve it by creating a defined function defaultResolve and using it in the default props but it's not working. Help is very welcome here :)

Notes

When resolve is an anonymous function (what can be very common) we may get an infinite loop as prevProps.resolve would never be equals to this.props.resolve.

For this reason I would suggest another solution instead. The component could expose a new prop to handle when the component should refetch. This way it would be more extensible and versatile with no extra code.

Something like:
shouldRefetch = (prevProps: GetProps<TData, TError>, prevState: GetState<TData, TError>) => boolean

The example in the issue #63 could be solved adding an extra component to handle changes in props (PrevProps):

export type ListUsersProps = Omit<GetProps<UserListResponse, {}>, "base" | "path"> & {
  tenantId: string;
  page: number;
  filter?: string;
};

export const ListUsers = ({ page, filter, ...props }: ListUsersProps) => (
  <PrevProps filter={filter} page={page}>{(prev, current) => (
    <Config>
      {({ getConfig }) => (
        <Get<UserListResponse>
          debounce
          base={getConfig("backend")}
          path={`${props.tenantId}/users?page=${page}`}
          resolve={(res: UserListResponse) => ({
            ...res,
            data: res.data.filter(user => (filter ? user.name.includes(filter) : true)),
          })}
          shouldRefetch={() => (
            prev.filter !== current.filter ||
            prev.page !== current.page
          )}
          {...props}
        />
      )}
    </Config>
  )}</PrevProps>
);

[fabien0102]

Hello @carlesba, thanks for your contribution 👍

I don't really understand your case about the infinity loop and arrow function 🤔

const a = () => "";
const b = a;
a === b; // true

Arrow function as any functions (anonymous or named) are compared by reference, so it should works as expected. I will try your branch locally to test this, this is very strange!

Also you have forgot the Poll component (that is almost the same as the Get)

[fabien0102]

I know why the Get › with debounce › should call the API only 10 times without debounce test is failing! It's definitetly not because the defaultProps, it's because we redefined the ResfulProvider in each rerender, so we have a new resolve function on each iteration 😉

restful-react/src/Context.tsx

Line 45 in 9064270

resolve: (data: any) => data,

So this is working for example (same resolve define in ResfulProvider):

it("should call the API only 10 times without debounce", async () => {
      let apiCalls = 0;
      nock("https://my-awesome-api.fake")
        .filteringPath(/test=[^&]*/g, "test=XXX")
        .get("/?test=XXX")
        .reply(200, () => ++apiCalls)
        .persist();

      const children = jest.fn();
      children.mockReturnValue(<div />);
      const resolve = a => a;

      const { rerender } = render(
        <RestfulProvider base="https://my-awesome-api.fake" resolve={resolve}>
          <Get path="?test=1">{children}</Get>
        </RestfulProvider>,
      );

      times(10, i =>
        rerender(
          <RestfulProvider base="https://my-awesome-api.fake" resolve={resolve}>
            <Get path={`?test=${i + 1}`}>{children}</Get>
          </RestfulProvider>,
        ),
      );

      expect(apiCalls).toEqual(10);
    });

This is just an issue for our tests, this behaviour should not exists on a real application 😉

We need to check if we can just rerender the Get instead of the all application, or we can just fix the resolve function reference as in my previous example (maybe with a little comment) I let you play around and take the best solution.

[carlesba]

I forgot about the Provider 😅 I'll change the tests. Thank you!

[carlesba]

Regarding Poll component, did you mean you would expect to update state not only when server response has changes but also when resolve has changed?

[TejasQ]

@carlesba let's try and maintain that it is identical in behavior to Get in terms of how its props work: meaning the answer to:

did you mean you would expect to update state not only when server response has changes but also when resolve has changed?

is that in terms of high-level behavior, it should use the new resolve function after it changes.

[carlesba]

I think this will make it. What do you think?

[TejasQ]

@carlesba looks good. I'll take a deeper look later today. Can I ask that you please remove the merge commit from master and instead rebase on the master? It helps us keep track of the project's evolution better that way.

[carlesba]

Sure, I'll rebase.

[fabien0102]

LGTM 👍

[TejasQ]

@carlesba THANK YOU for your contribution! You have made our project so much better!