Merge pull request #132 from dogfessional/fix-touch-event-cleanup

Fix touch event cleanup

Author: hartzis

package-lock.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",
@@ -21,7 +21,7 @@
   },
   "size-limit": [
     {
-      "limit": "1.8 KB",
+      "limit": "1.875 KB",
       "path": "es/index.js"
     }
   ],

package.json

@@ -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)
+    })
   })
 })
 

src/__tests__/index.spec.js

@@ -112,22 +112,24 @@ 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 => {
-    if (el && el.removeEventListener) {
-      el.removeEventListener(touchStart, onStart)
-      el.removeEventListener(touchMove, onMove)
-      el.removeEventListener(touchEnd, onUp)
+  const attachTouch = el => {
+    if (el && el.addEventListener) {
+      // 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))
     }
   }
 
@@ -138,22 +140,39 @@ 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)
+
+      let addState = {}
+      // if new DOM el clean up old DOM and reset cleanUpTouch
+      if (state.el && state.el !== el && state.cleanUpTouch) {
+        state.cleanUpTouch()
+        addState.cleanUpTouch = null
+      }
       // 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 && el) {
+        addState.cleanUpTouch = attachTouch(el)
       }
-      return state
+
+      // store event attached DOM el for comparison, clean up, and re-attachment
+      return { ...state, el, ...addState }
     })
   }
 
+  // update state, props, and handlers
+  set(state => {
+    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, props, ...addState }
+  })
+
   // set ref callback to attach touch event listeners
   const output = { ref: onRef }
 
@@ -162,9 +181,6 @@ function getHandlers(set, props) {
     output.onMouseDown = onStart
   }
 
-  // update props
-  set(state => ({ ...state, props }))
-
   return output
 }