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.

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

How to use the rerender API ? #18

Closed
DavidBabel opened this issue Mar 21, 2019 · 11 comments
Closed

How to use the rerender API ? #18

DavidBabel opened this issue Mar 21, 2019 · 11 comments
Labels
question Further information is requested

Comments

@DavidBabel
Copy link

DavidBabel commented Mar 21, 2019

Hey guys,

I'm sorry, i'm not very confortable with other testing libraries, and i cannot get how to test my hooks with this library.

I have a very simple hook, which wait for visibility to store a timestamp of render time, which will never update after that :

export function useRenderedAt(isViewable: boolean) {
  const [renderedAt, setRenderedAt] = React.useState(Infinity);

  React.useEffect(() => {
    if (isViewable) {
      setRenderedAt(Date.now());
    }
  }, [isViewable]);

  return renderedAt;
}

I'm trying to test it this way :

  test.only('should return render time when viewable', done => {
    const hook = renderHook(() => useRenderedAt(false), {
      initialProps: false
    });
    expect(hook.result.current).toBe(Infinity);
    hook.waitForNextUpdate().then(() => {
      expect(hook.result.current).not.toBe(Infinity);
      done();
    });
    hook.rerender(true);
  });

What am i doing wrong ?

@DavidBabel DavidBabel added the question Further information is requested label Mar 21, 2019
@mpeyper
Copy link
Member

mpeyper commented Mar 21, 2019

Hi @DavidBabel (great name for JS dev!),

You're so close with your test. The issue is that the props are not passed through the the hook callback so it is using false every time the hook renders, despite your providing new props to the hook. Change the first line of your test to

    const hook = renderHook((isVisible) => useRenderedAt(isVisible), {
      initialProps: false
    });

allows the test to pass.

We should document this better(apparently this is not documented at all).

@DavidBabel
Copy link
Author

DavidBabel commented Mar 21, 2019

We should document this better(apparently this is not documented at all).

Yeah that's why i was a bit confused about that. Thanks for your answer, it totally makes sense now. I did not have the energy to check in the code yesterday :p

Great project anyway

Edit: I actually reopened. Do i have to use waitForNextUpdate in my case ?
Or this is enough ? :

    const hook = renderHook(isVisible => useRenderedAt(isVisible), {
      initialProps: false
    });
    hook.rerender(true);
    const timestamp = hook.result.current;
    expect(timestamp).not.toBe(Infinity);
    hook.rerender(true);
    expect(hook.result.current).toBe(timestamp);

@DavidBabel
Copy link
Author

Ok i find out. So for googlers, the answer is no. Ne need for async since the hooks is synchrone.

so this works perfectly :

  test('should return render time when viewable and never reupdate it', () => {
    const hook = renderHook(isVisible => useRenderedAt(isVisible), {
      initialProps: false
    });
    hook.rerender(true);
    const timestamp = hook.result.current;
    expect(timestamp).not.toBe(Infinity);
    hook.rerender(true);
    expect(hook.result.current).toBe(timestamp);
    hook.rerender(false);
    expect(hook.result.current).toBe(timestamp);
  });

@mpeyper mpeyper mentioned this issue Mar 21, 2019
22 tasks
@goldo
Copy link

goldo commented Apr 17, 2019

@DavidBabel thanks for writing this down.

I find this rerender API not intuitive and complicated for something like that :(

@DavidBabel
Copy link
Author

Glad it helps

@mpeyper
Copy link
Member

mpeyper commented Apr 27, 2019

@goldo, I'd be interested in hearing your thoughts in #56 about why you find this rerender API not intuitive and complicated.

@DavidBabel you're welcome to leave your thoughts too :)

@goldo
Copy link

goldo commented Apr 27, 2019

@mpeyper I think mainly because you have to recreate a function and map the params of it to the params of the function of your hook.
Ex:

  test('useMyHook', () => {
    const hook = renderHook(number=> useMyHook({timeout, ref, number}), {
      initialProps: false
    });
    hook.rerender(5);
  });

Here, hook.rerender(5) is not explicit at all and you have to learn and adapt your mapping test function.

I guess It would have been better to keep the exact same function signature for example ?

@mpeyper
Copy link
Member

mpeyper commented Apr 27, 2019

Here, hook.rerender(5) is not explicit at all and you have to learn and adapt your mapping test function.

Generally, and what the examples in the new draft docs have, I would use an object for the props so that you can name the value to add a bit of traceability to the value and it's usage

  test('useMyHook', () => {
    const hook = renderHook(({ number }) => useMyHook({timeout, ref, number}), {
      initialProps: { number: false }
    });
    hook.rerender({ number: 5 });
  });

But I totally understand that this still is not as explicit as you might want. It's probably a case of it being apparent to me, the author of the library, but not so much for fresh eyes seeing the API for the first time.

I guess It would have been better to keep the exact same function signature for example ?

You mean like

  test('useMyHook', () => {
    const { rerender } = renderHook(() => useMyHook({timeout, ref, false}));
    rerender(() => useMyHook({timeout, ref, 5}));
  });

or

  test('useMyHook', () => {
    renderHook(() => useMyHook({timeout, ref, false}));
    renderHook(() => useMyHook({timeout, ref, 5}));
  });

Or something else?

@goldo
Copy link

goldo commented Apr 27, 2019

test('useMyHook', () => {
    const hook = renderHook(()=> useMyHook({timeout, ref, number}));
    hook.rerender({ number });
  });

something like this ?

@mpeyper
Copy link
Member

mpeyper commented Apr 28, 2019

I would like to move this discussion to #56, but I'll answer here for continuity.

something like this ?

I'm not sure where number gets defined or updated to understand the usage of that one.

if its:

test('useMyHook', () => {
  const timeout = 100 // or whatever
  const ref = { current: '' } // or whatever
  let number = false

  const hook = renderHook(()=> useMyHook({ timeout, ref, number }));

  number = 5
  hook.rerender({ number });
});

Then passing the updated value to rerender is unnecessary as the hook callback will already pick up the new value when rerendering, so it could just be:

test('useMyHook', () => {
  const timeout = 100 // or whatever
  const ref = { current: '' } // or whatever
  let number = false

  const hook = renderHook(()=> useMyHook({ timeout, ref, number }));

  number = 5
  hook.rerender();
});

This would work with the current API.

If the intention was something more like:

test('useMyHook', () => {
  const timeout = 100 // or whatever
  const ref = { current: '' } // or whatever

  const hook = renderHook(()=> useMyHook({ timeout, ref, number: false }));

  hook.rerender({ number: 5 });
});

Then I don't think this is possible as the hook callback has no idea about the value passed to rerender, which is why the current implementation has initialProps that get passed into the hook callback, so it can update them with the incoming values from rerender.

I still think I may be completely misunderstanding what you are saying though.

@goldo
Copy link

goldo commented May 2, 2019

@mpeyper sorry for the delay
Yes I though more about your last example, it seems to be the more obvious and readable to me. I didn't think about it more deeply in its implementation, I didn't check the code neither, so my bad, but I guess there should be a better way anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants