Skip to content

Memoize hook output #149

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 15, 2019
Merged

Memoize hook output #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36e8b01
chore(lint): add eslint-plugin-react-hooks
FaberVitale Jun 2, 2019
e99b51b
refactor(performance): memoize useSwipeable output
FaberVitale Jun 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@
"react/jsx-no-undef": 2,
"react/jsx-wrap-multilines": 2,
"react/no-string-refs": 0,
"react/prop-types": [1, {"ignore": ["className", "children", "style"]}]
"react/prop-types": [1, { "ignore": ["className", "children", "style"] }],
"react-hooks/rules-of-hooks": 2,
"react-hooks/exhaustive-deps": 1
},
"plugins": [
"import",
"react"
]
"plugins": ["import", "react", "react-hooks"]
}
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
"eslint-plugin-import": "^2.14.0",
"eslint-plugin-prettier": "^3.0.0",
"eslint-plugin-react": "^7.11.1",
"eslint-plugin-react-hooks": "^1.6.0",
"gh-pages": "^1.0.0",
"jest": "^23.6.0",
"prettier": "1.15.3",
Expand Down
56 changes: 49 additions & 7 deletions src/__tests__/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* global document, jest, expect, beforeAll, afterAll */
import React from 'react'
import Enzyme from 'enzyme'
import PropTypes from 'prop-types'
import Adapter from 'enzyme-adapter-react-16'
import { Swipeable, useSwipeable, LEFT, RIGHT, UP, DOWN } from '../index'
import { createTouchEventObject as cte, createMouseEventObject as cme } from './helpers/events'
Expand Down Expand Up @@ -55,14 +56,22 @@ function mockListenersSetup(el) {
*/
function SwipeableUsingHook(props) {
const eventHandlers = useSwipeable(props)
const Elem = props.nodeName
// Use innerRef prop to access the mounted div for testing.
const ref = el => (props.innerRef && props.innerRef(el), eventHandlers.ref(el)) // eslint-disable-line
return (
<div {...eventHandlers} ref={ref}>
<Elem {...eventHandlers} ref={ref}>
{props.children}
</div>
</Elem>
)
}
SwipeableUsingHook.propTypes = {
nodeName: PropTypes.string
}

SwipeableUsingHook.defaultProps = {
nodeName: 'div'
}

function setupGetMountedComponent(TYPE, mockListeners = mockListenersSetup) {
return props => {
Expand Down Expand Up @@ -387,7 +396,7 @@ function setupGetMountedComponent(TYPE, mockListeners = mockListenersSetup) {
})
})

it('Cleans up and re-attaches touch event listeners', () => {
it('Cleans up and re-attaches touch event listeners if trackTouch changes', () => {
let spies
const mockListeners = el => {
// already spying
Expand All @@ -413,6 +422,32 @@ function setupGetMountedComponent(TYPE, mockListeners = mockListenersSetup) {
expect(spies.addEventListener).toHaveBeenCalledTimes(6)
expect(spies.removeEventListener).toHaveBeenCalledTimes(3)
})

it('Cleans up and re-attaches touch event listeners if the DOM element changes', () => {
let spies
const mockListeners = el => {
// already spying
if (spies) return
spies = {}
spies.addEventListener = jest.spyOn(el, 'addEventListener')
spies.removeEventListener = jest.spyOn(el, 'removeEventListener')
}
const { wrapper } = setupGetMountedComponent(TYPE, mockListeners)({})

expect(spies.addEventListener).toHaveBeenCalledTimes(3)
expect(spies.removeEventListener).not.toHaveBeenCalled()

wrapper.setProps({ nodeName: 'p' })

expect(spies.addEventListener).toHaveBeenCalledTimes(3)
expect(spies.removeEventListener).toHaveBeenCalledTimes(3)
// VERIFY REMOVED HANDLERS ARE THE SAME ONES THAT WERE ADDED!
expect(spies.addEventListener.mock.calls.length).toBe(3)
spies.addEventListener.mock.calls.forEach((call, idx) => {
expect(spies.removeEventListener.mock.calls[idx][0]).toBe(call[0])
expect(spies.removeEventListener.mock.calls[idx][1]).toBe(call[1])
})
})
})

it(`${TYPE} handles updated prop swipe callbacks`, () => {
Expand All @@ -423,15 +458,22 @@ function setupGetMountedComponent(TYPE, mockListeners = mockListenersSetup) {
}
const onSwipedLeft = jest.fn()

function TestHookComponent({ next }) {
const handlers = useSwipeable({ onSwipedLeft: next })
// Use innerRef to access the mounted div for testing.
const ref = el => (innerRef(el), handlers.ref(el))
return <div {...handlers} ref={ref} />
}
TestHookComponent.propTypes = {
next: PropTypes.func.isRequired
}

function TestComponent() {
const [page, setPage] = React.useState(0)
const next = () => (setPage(page + 1), onSwipedLeft(page + 1))

if (TYPE === USESWIPEABLE) {
const handlers = useSwipeable({ onSwipedLeft: next })
// Use innerRef to access the mounted div for testing.
const ref = el => (innerRef(el), handlers.ref(el))
return <div {...handlers} ref={ref} />
return <TestHookComponent next={next} />
}
if (TYPE === SWIPEABLE) {
// Use innerRef to access the mounted div for testing.
Expand Down
56 changes: 35 additions & 21 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,22 +161,6 @@ function getHandlers(set, handlerProps) {
})
}

// update state, and handlers
set((state, props) => {
let addState = {}
// clean up touch handlers if no longer tracking touches
if (!props.trackTouch && state.cleanUpTouch) {
state.cleanUpTouch()
addState.cleanUpTouch = null
} else if (props.trackTouch && !state.cleanUpTouch) {
// attach/re-attach touch handlers
if (state.el) {
addState.cleanUpTouch = attachTouch(state.el)
}
}
return { ...state, ...addState }
})

// set ref callback to attach touch event listeners
const output = { ref: onRef }

Expand All @@ -185,17 +169,46 @@ function getHandlers(set, handlerProps) {
output.onMouseDown = onStart
}

return output
return [output, attachTouch]
}

function updateTransientState(state, props, attachTouch) {
let addState = {}
// clean up touch handlers if no longer tracking touches
if (!props.trackTouch && state.cleanUpTouch) {
state.cleanUpTouch()
addState.cleanUpTouch = null
} else if (props.trackTouch && !state.cleanUpTouch) {
// attach/re-attach touch handlers
if (state.el) {
addState.cleanUpTouch = attachTouch(state.el)
}
}
return { ...state, ...addState }
}

export function useSwipeable(props) {
const { trackMouse } = props
const transientState = React.useRef({ ...initialState, type: 'hook' })
const transientProps = React.useRef()
transientProps.current = { ...defaultProps, ...props }
return getHandlers(
cb => (transientState.current = cb(transientState.current, transientProps.current)),
{ trackMouse: props.trackMouse }

const [handlers, attachTouch] = React.useMemo(
() =>
getHandlers(
cb => (transientState.current = cb(transientState.current, transientProps.current)),
{ trackMouse }
),
[trackMouse]
)

transientState.current = updateTransientState(
transientState.current,
transientProps.current,
attachTouch
)

return handlers
}

export class Swipeable extends React.PureComponent {
Expand Down Expand Up @@ -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 })
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

this.transientState = updateTransientState(this.transientState, this.props, attachTouch)
const ref = innerRef ? el => (innerRef(el), handlers.ref(el)) : handlers.ref
return React.createElement(nodeName, { ...handlers, className, style, ref }, children)
}
Expand Down