Skip to content

Conversation

@stephencookdev
Copy link

Currently different props objects are sent to getHandlers on each render.

In theory this is fine, but because of the way that getHandlers sets up event listeners (such as onMove listeners), the state.props object that gets used by these listeners is actually fixed as an older value.

In practice, what this means, is that if props like preventDefaultTouchmoveEvent change on the fly, then the value of state.props.preventDefaultTouchmoveEvent in the onMove callback does not get the new value, and instead keeps the initial value sent through.

This PR sets up an explicit props object to be passed through - so that the object reference is the same everywhere, so that the callback handlers get the up to date prop values.

…e through to e.g. the touch listener callback
@hartzis
Copy link
Collaborator

hartzis commented Apr 12, 2019

@stephencookdev this is absolutely a valid concern and thank you for taking a look and setting up a PR. But I do think this may have been fixed recently?

I remember dealing with this issue and my concerns when working on #131 , and think it was fixed with this addition then updated in #132 here

If you still think it is an issue could you add a test that would fail before this PRs additions? That would be incredibly helpful. Thank you again!

@hartzis hartzis self-requested a review April 12, 2019 16:34
Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a test that would fail before this PRs additions?

@stephencookdev
Copy link
Author

Definitely still an issue, I think! But yes, I'll work on a failing test on Monday morning 🙂

@hartzis
Copy link
Collaborator

hartzis commented Apr 18, 2019

@stephencookdev does #138 , published as react-swipeable@5.2.0-alpha.4 fix the issue for you?

Updated sandbox to try - https://codesandbox.io/s/9oy62kp0xr

@hartzis
Copy link
Collaborator

hartzis commented May 10, 2019

Should be fixed from #138 and released as 5.2.0

@stephencookdev Please let me know if this is not the case. Thank you again for the PR and for bringing the issue to my attention!

@hartzis hartzis closed this May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants