-
Notifications
You must be signed in to change notification settings - Fork 147
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
Memoize hook output #149
Memoize hook output #149
Conversation
Changes: 1. moved side effects performed by getHandlers to a new function called updateTransientState. 2. getHandlers output is memoized. 3. Added a test. Links: * useMemo: https://reactjs.org/docs/hooks-reference.html#usememo
@FaberVitale This is awesome! Thank you for endeavoring an attempt at #134. I really like it after my initial glance. I'd like to dig a bit more into this attempt this week and pull down the branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial glance 👍
@@ -228,7 +241,8 @@ export class Swipeable extends React.PureComponent { | |||
|
|||
render() { | |||
const { className, style, nodeName = 'div', innerRef, children, trackMouse } = this.props | |||
const handlers = getHandlers(this._set, { trackMouse }) | |||
const [handlers, attachTouch] = getHandlers(this._set, { trackMouse }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Thoughts on memoizing the class component?
Could add handlers
and attachTouch
to this
and set in the constructor initially, and add a componentDidUpdate
that only re-runs getHandlers
if trackMouse
changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work flawlessly.
Let me try implement this enhancement:
if it works, I'll push the changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache handlers
We should use getDerivedStateFromProps
instead of componentDidUpdate
to preserve
the same order of operations between the hook version and the component version:
componentDidUpdate
runs in the commit phase while all the useSwipeable
code runs in the render phase(see).
Here's a snippet:
[...]
static getDerivedStateFromProps({ trackMouse }, { trackMouse: prevTrackMouse, _set }) {
if (prevTrackMouse !== trackMouse) {
const [handlers, attachTouch] = getHandlers(_set, { trackMouse })
return { trackMouse, handlers, attachTouch }
}
return null
}
[...]
SSR
We should add 1 or 2 SSR tests but I think it is outside the scope of this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a version that uses getDerivedStateFromProps in this branch.
I've noticed that this packages has React >= 16.0.0 as peerDependency but getDerivedStateFromProps
got added in v16.3.
You'd probably have to bump to a major version if you want to either use getDerivedStateFromProps
or move the logic of both hook and component logic from the render phase to the commit one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with getDerivedStatefromProps
and really like your implementation.
Release a major to include the class-based improvements is definitely the right way along with bumping the required minimum version of react.
With your guidance and example implementation, I think I'm leaning towards holding off, possibly indefinitely, on the including the class-based improvements. As hooks are the path forward I think the next major of this package should probably deprecate the component and only export a hook.
Let's just move forward with this amazing addition to the hook! Thank you @FaberVitale!
Changes:
fixes useSwipeable improvements #134
adds eslint-plugin-react-hooks