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

useSwipeable does not refresh callbacks #136

Closed
caesarsol opened this issue Apr 16, 2019 · 8 comments
Closed

useSwipeable does not refresh callbacks #136

caesarsol opened this issue Apr 16, 2019 · 8 comments
Assignees
Labels

Comments

@caesarsol
Copy link

Describe the bug

It seems that useSwipable does not refresh the handlers on subsequent renders.
It appears that the handlers used are the first ones with which the function is called.

Maybe it would need a dependency array, like useImperativeHandle?

(If this behaviour is expected, we should maybe write a description in the README)

Steps or Sandbox to reproduce

https://codesandbox.io/s/p9n13oo0x

Try to swipe two times in the red area, the Page number does not update as expected.
In the green area it works, because we are calling the handlers inside a setTimeout() with closured value of the page variable.

Device/Browser

All.

@bas-l
Copy link

bas-l commented Apr 16, 2019

I have the same issue. Upon swiping I do a callback that updates a prop in the parent, and the prop is updated in the render function, but not in the handler. Is it possible this would also be fixed by #135?

@caesarsol caesarsol changed the title useSwipable does not refresh callbacks useSwipeable does not refresh callbacks Apr 16, 2019
@hartzis hartzis self-assigned this Apr 17, 2019
@hartzis
Copy link
Collaborator

hartzis commented Apr 17, 2019

@caesarsol Thank you for the issue and the example, incredibly helpful being able to reproduce.

Great question about if this is expected or not. I'm intrigued by the useImperativeHandle and may be related to my issue #134 .

@bas-l I do think it is related to #135 too, but that is for the component, <Swipeable/>, specifically and not the hook.

I'm going to dig a bit more into this. And please let me know if you think you have a solution or thoughts on if this should or shouldn't be expected.

@hartzis
Copy link
Collaborator

hartzis commented Apr 17, 2019

@caesarsol @bas-l @stephencookdev It is late, but i may have solved it? It may not be elegant but i think it works. #138 published as react-swipeable@5.2.0-alpha.1

@caesarsol I forked your codesandbox, again THANK YOU, and with 5.2.0-alpha.1 it appears to be working, https://codesandbox.io/s/9oy62kp0xr

@bas-l
Copy link

bas-l commented Apr 17, 2019

@hartzis Thanks for the quick update! Only now Typescript cannot find the import { useSwipeable } from 'react-swipeable'; anymore.

TypeScript error: Module '"<shortened>/node_modules/react-swipeable/types"' has no exported member 'useSwipeable'

Oh I only now see #137. Waiting for that now I guess :-) Going back to 5.1.0 for now.

@hartzis
Copy link
Collaborator

hartzis commented Apr 17, 2019

@bas-l I just merged #137 , funny i just made that last night too.

I just cut another alpha, react-swipeable@5.2.0-alpha.3 that includes the useSwipeable types.

@caesarsol
Copy link
Author

Hi @hartzis, thanks for the very quick response and fix!!

Very happy to have been useful. :)

@bas-l
Copy link

bas-l commented Apr 23, 2019

Now running 5.2.0-alpha.4 and everything seems to run well. Thanks @hartzis!

@hartzis
Copy link
Collaborator

hartzis commented Apr 23, 2019

fix published with 5.2.0 thank you @caesarsol and @bas-l !

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

No branches or pull requests

3 participants