From bde6a237963aa4308b9e728913d0aaec3a8a4960 Mon Sep 17 00:00:00 2001 From: Brian Emil Hartz Date: Wed, 20 Mar 2019 21:32:28 -0600 Subject: [PATCH 1/5] add test for touch event listener cleanup --- src/__tests__/index.spec.js | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/__tests__/index.spec.js b/src/__tests__/index.spec.js index a21bcca4..93e4e688 100644 --- a/src/__tests__/index.spec.js +++ b/src/__tests__/index.spec.js @@ -35,7 +35,7 @@ const expectSwipeFuncsDir = (sf, dir) => } }) -function mockListenerSetup(el) { +function mockListenersSetup(el) { // track eventListener adds to trigger later // idea from - https://github.com/airbnb/enzyme/issues/426#issuecomment-228601631 const eventListenerMap = {} @@ -62,13 +62,13 @@ function SwipeableUsingHook(props) { ) } -function setupGetMountedComponent(type) { +function setupGetMountedComponent(type, mockListeners = mockListenersSetup) { return props => { let wrapper let eventListenerMap const innerRef = el => { // don't re-assign eventlistener map - if (!eventListenerMap) eventListenerMap = mockListenerSetup(el) + if (!eventListenerMap) eventListenerMap = mockListeners(el) } if (type === 'Swipeable') { wrapper = mount( @@ -100,7 +100,7 @@ function setupGetMountedComponent(type) { beforeEach(() => { // track eventListener adds to trigger later // idea from - https://github.com/airbnb/enzyme/issues/426#issuecomment-228601631 - eventListenerMapDocument = mockListenerSetup(document) + eventListenerMapDocument = mockListenersSetup(document) }) afterAll(() => { document.eventListener = origAddEventListener @@ -381,6 +381,33 @@ function setupGetMountedComponent(type) { expect(swipeFuncs[`onSwiped${dir}`]).not.toHaveBeenCalled() }) }) + + it('Cleans up and re-attachs touch event listeners', () => { + 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({ trackTouch: false }) + 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]) + }) + + wrapper.setProps({ trackTouch: true }) + expect(spies.addEventListener).toHaveBeenCalledTimes(6) + expect(spies.removeEventListener).toHaveBeenCalledTimes(3) + }) }) }) From c44a5006b3808331c9733d495ca9888df45d13d0 Mon Sep 17 00:00:00 2001 From: Brian Emil Hartz Date: Wed, 20 Mar 2019 22:13:48 -0600 Subject: [PATCH 2/5] fix touch event cleanup --- src/index.js | 57 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/src/index.js b/src/index.js index 4c07b01e..a8038b13 100644 --- a/src/index.js +++ b/src/index.js @@ -22,6 +22,7 @@ export const DOWN = 'Down' const touchStart = 'touchstart' const touchMove = 'touchmove' const touchEnd = 'touchend' +const touchEvents = [touchStart, touchMove, touchEnd] const mouseMove = 'mousemove' const mouseUp = 'mouseup' @@ -112,22 +113,28 @@ function getHandlers(set, props) { }) } - const stop = () => { + const cleanUpMouse = () => { // safe to just call removeEventListener document.removeEventListener(mouseMove, onMove) document.removeEventListener(mouseUp, onUp) } const onUp = e => { - stop() + cleanUpMouse() onEnd(e) } - const cleanUp = el => { + const attachTouch = el => { + if (el && el.addEventListener) { + const attach = { [touchStart]: onStart, [touchMove]: onMove, [touchEnd]: onUp } + touchEvents.forEach(e => el.addEventListener(e, attach[e])) + return getCleanUpTouch(el, attach) + } + } + + const getCleanUpTouch = (el, attached) => () => { if (el && el.removeEventListener) { - el.removeEventListener(touchStart, onStart) - el.removeEventListener(touchMove, onMove) - el.removeEventListener(touchEnd, onUp) + touchEvents.forEach(e => el.removeEventListener(e, attached[e])) } } @@ -138,19 +145,19 @@ function getHandlers(set, props) { set(state => { // if the same DOM el as previous just return state if (state.el === el) return state - // if new DOM el clean up old DOM - if (state.el && state.el !== el) cleanUp(state.el) + + // store event attached DOM el for comparison, clean up, and re-attachment + const newState = { ...state, el } + + // if new DOM el clean up old DOM and reset cleanUpTouch + if (state.el && state.el !== el && state.cleanUpTouch) { + newState.cleanUpTouch = state.cleanUpTouch() + } // only attach if we want to track touch - if (state.props.trackTouch) { - if (el && el.addEventListener) { - el.addEventListener(touchStart, onStart) - el.addEventListener(touchMove, onMove) - el.addEventListener(touchEnd, onUp) - // store event attached DOM el for comparison - return { ...state, el } - } + if (state.props.trackTouch && newState.el) { + newState.cleanUpTouch = attachTouch(newState.el) } - return state + return newState }) } @@ -162,8 +169,20 @@ function getHandlers(set, props) { output.onMouseDown = onStart } - // update props - set(state => ({ ...state, props })) + // update state, props, and handlers + set(state => { + const newState = { ...state, props } + // clean up touch handlers if no longer tracking touches + if (!props.trackTouch && state.cleanUpTouch) { + newState.cleanUpTouch = state.cleanUpTouch() + } else if (props.trackTouch && !state.cleanUpTouch) { + // attach/re-attach touch handlers + if (newState.el) { + newState.cleanUpTouch = attachTouch(newState.el) + } + } + return newState + }) return output } From 5929ec99ed9eef081cee6d2043b9a470bf0c20bb Mon Sep 17 00:00:00 2001 From: Brian Emil Hartz Date: Wed, 20 Mar 2019 22:37:22 -0600 Subject: [PATCH 3/5] bump size-limit with room to breath --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 3da9b5d8..fbbb4a06 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ }, "size-limit": [ { - "limit": "1.8 KB", + "limit": "1.875 KB", "path": "es/index.js" } ], From bb9b6df0b8f1bae55fdcb3b2c46eb599905e2039 Mon Sep 17 00:00:00 2001 From: Brian Emil Hartz Date: Wed, 20 Mar 2019 22:38:02 -0600 Subject: [PATCH 4/5] 5.1.0-alpha.2 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 79fb9871..39402bde 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "react-swipeable", - "version": "5.1.0-alpha.1", + "version": "5.1.0-alpha.2", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index fbbb4a06..4e055f09 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-swipeable", - "version": "5.1.0-alpha.1", + "version": "5.1.0-alpha.2", "description": "Swipe and touch handlers for react", "main": "./lib/index.js", "module": "es/index.js", From edd83878a4f7c6ee12b8d8f337c525e5b383ac4b Mon Sep 17 00:00:00 2001 From: Brian Emil Hartz Date: Thu, 21 Mar 2019 18:30:39 -0600 Subject: [PATCH 5/5] clean ups and attempt to simplify --- src/index.js | 57 +++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/src/index.js b/src/index.js index a8038b13..7fb2ffb1 100644 --- a/src/index.js +++ b/src/index.js @@ -22,7 +22,6 @@ export const DOWN = 'Down' const touchStart = 'touchstart' const touchMove = 'touchmove' const touchEnd = 'touchend' -const touchEvents = [touchStart, touchMove, touchEnd] const mouseMove = 'mousemove' const mouseUp = 'mouseup' @@ -126,15 +125,11 @@ function getHandlers(set, props) { const attachTouch = el => { if (el && el.addEventListener) { - const attach = { [touchStart]: onStart, [touchMove]: onMove, [touchEnd]: onUp } - touchEvents.forEach(e => el.addEventListener(e, attach[e])) - return getCleanUpTouch(el, attach) - } - } - - const getCleanUpTouch = (el, attached) => () => { - if (el && el.removeEventListener) { - touchEvents.forEach(e => el.removeEventListener(e, attached[e])) + // attach touch event listeners and handlers + const tls = [[touchStart, onStart], [touchMove, onMove], [touchEnd, onEnd]] + tls.forEach(([e, h]) => el.addEventListener(e, h)) + // return properly scoped cleanup method for removing listeners + return () => tls.forEach(([e, h]) => el.removeEventListener(e, h)) } } @@ -146,44 +141,46 @@ function getHandlers(set, props) { // if the same DOM el as previous just return state if (state.el === el) return state - // store event attached DOM el for comparison, clean up, and re-attachment - const newState = { ...state, el } - + let addState = {} // if new DOM el clean up old DOM and reset cleanUpTouch if (state.el && state.el !== el && state.cleanUpTouch) { - newState.cleanUpTouch = state.cleanUpTouch() + state.cleanUpTouch() + addState.cleanUpTouch = null } // only attach if we want to track touch - if (state.props.trackTouch && newState.el) { - newState.cleanUpTouch = attachTouch(newState.el) + if (state.props.trackTouch && el) { + addState.cleanUpTouch = attachTouch(el) } - return newState - }) - } - - // set ref callback to attach touch event listeners - const output = { ref: onRef } - // if track mouse attach mouse down listener - if (props.trackMouse) { - output.onMouseDown = onStart + // store event attached DOM el for comparison, clean up, and re-attachment + return { ...state, el, ...addState } + }) } // update state, props, and handlers set(state => { - const newState = { ...state, props } + let addState = {} // clean up touch handlers if no longer tracking touches if (!props.trackTouch && state.cleanUpTouch) { - newState.cleanUpTouch = state.cleanUpTouch() + state.cleanUpTouch() + addState.cleanUpTouch = null } else if (props.trackTouch && !state.cleanUpTouch) { // attach/re-attach touch handlers - if (newState.el) { - newState.cleanUpTouch = attachTouch(newState.el) + if (state.el) { + addState.cleanUpTouch = attachTouch(state.el) } } - return newState + return { ...state, props, ...addState } }) + // set ref callback to attach touch event listeners + const output = { ref: onRef } + + // if track mouse attach mouse down listener + if (props.trackMouse) { + output.onMouseDown = onStart + } + return output }