From 130830c19fb7d2715367d51502d6c052bcf95d85 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Fri, 9 Nov 2018 09:44:04 -0800 Subject: [PATCH] Merge pull request #17 from slorber/navigation-events-bug Fix NavigationEvents implementation --- packages/core/src/views/NavigationEvents.js | 43 +-- .../views/__tests__/NavigationEvents-test.js | 286 +++++++----------- 2 files changed, 122 insertions(+), 207 deletions(-) diff --git a/packages/core/src/views/NavigationEvents.js b/packages/core/src/views/NavigationEvents.js index 84d7e918..2de5424d 100644 --- a/packages/core/src/views/NavigationEvents.js +++ b/packages/core/src/views/NavigationEvents.js @@ -11,43 +11,30 @@ const EventNameToPropName = { const EventNames = Object.keys(EventNameToPropName); class NavigationEvents extends React.Component { + getPropListener = eventName => this.props[EventNameToPropName[eventName]]; + componentDidMount() { this.subscriptions = {}; - EventNames.forEach(this.addListener); - } - componentDidUpdate(prevProps) { + // We register all navigation listeners on mount to ensure listener stability across re-render + // A former implementation was replacing (removing/adding) listeners on all update (if prop provided) + // but there were issues (see https://github.com/react-navigation/react-navigation/issues/5058) EventNames.forEach(eventName => { - const listenerHasChanged = - this.props[EventNameToPropName[eventName]] !== - prevProps[EventNameToPropName[eventName]]; - if (listenerHasChanged) { - this.removeListener(eventName); - this.addListener(eventName); - } + this.subscriptions[eventName] = this.props.navigation.addListener( + eventName, + (...args) => { + const propListener = this.getPropListener(eventName); + return propListener && propListener(...args); + } + ); }); } componentWillUnmount() { - EventNames.forEach(this.removeListener); - } - - addListener = eventName => { - const listener = this.props[EventNameToPropName[eventName]]; - if (listener) { - this.subscriptions[eventName] = this.props.navigation.addListener( - eventName, - listener - ); - } - }; - - removeListener = eventName => { - if (this.subscriptions[eventName]) { + EventNames.forEach(eventName => { this.subscriptions[eventName].remove(); - this.subscriptions[eventName] = undefined; - } - }; + }); + } render() { return null; diff --git a/packages/core/src/views/__tests__/NavigationEvents-test.js b/packages/core/src/views/__tests__/NavigationEvents-test.js index 6108447f..9a6d393d 100644 --- a/packages/core/src/views/__tests__/NavigationEvents-test.js +++ b/packages/core/src/views/__tests__/NavigationEvents-test.js @@ -3,241 +3,169 @@ import renderer from 'react-test-renderer'; import NavigationEvents from '../NavigationEvents'; import NavigationContext from '../NavigationContext'; -const createListener = () => () => {}; +const createPropListener = () => jest.fn(); // An easy way to create the 4 listeners prop const createEventListenersProp = () => ({ - onWillFocus: createListener(), - onDidFocus: createListener(), - onWillBlur: createListener(), - onDidBlur: createListener(), + onWillFocus: createPropListener(), + onDidFocus: createPropListener(), + onWillBlur: createPropListener(), + onDidBlur: createPropListener(), }); -const createNavigationAndHelpers = () => { - // A little API to spy on subscription remove calls that are performed during the tests - const removeCallsAPI = (() => { - let removeCalls = []; +const createTestNavigationAndHelpers = () => { + const NavigationListenersAPI = (() => { + let listeners = { + willFocus: [], + didFocus: [], + willBlur: [], + didBlur: [], + }; return { - reset: () => { - removeCalls = []; + add: (eventName, handler) => { + listeners[eventName].push(handler); }, - add: (name, handler) => { - removeCalls.push({ name, handler }); + remove: (eventName, handler) => { + listeners[eventName] = listeners[eventName].filter(h => h !== handler); }, - checkRemoveCalled: count => { - expect(removeCalls.length).toBe(count); + get: eventName => { + return listeners[eventName]; }, - checkRemoveCalledWith: (name, handler) => { - expect(removeCalls).toContainEqual({ name, handler }); + call: eventName => { + listeners[eventName].forEach(listener => listener()); }, }; })(); const navigation = { - addListener: jest.fn((name, handler) => { + addListener: jest.fn((eventName, handler) => { + NavigationListenersAPI.add(eventName, handler); return { - remove: () => removeCallsAPI.add(name, handler), + remove: () => NavigationListenersAPI.remove(eventName, handler), }; }), }; - const checkAddListenerCalled = count => { - expect(navigation.addListener).toHaveBeenCalledTimes(count); - }; - const checkAddListenerCalledWith = (eventName, handler) => { - expect(navigation.addListener).toHaveBeenCalledWith(eventName, handler); - }; - const checkRemoveCalled = count => { - removeCallsAPI.checkRemoveCalled(count); - }; - const checkRemoveCalledWith = (eventName, handler) => { - removeCallsAPI.checkRemoveCalledWith(eventName, handler); - }; - return { navigation, - removeCallsAPI, - checkAddListenerCalled, - checkAddListenerCalledWith, - checkRemoveCalled, - checkRemoveCalledWith, + NavigationListenersAPI, }; }; -// We test 2 distinct ways to provide the navigation to the NavigationEvents (prop/context) -const NavigationEventsTestComp = ({ - withContext = true, - navigation, - ...props -}) => { - if (withContext) { - return ( - - - - ); - } else { - return ; - } -}; - describe('NavigationEvents', () => { - it('add all listeners with navigation prop', () => { + it('add all listeners on mount and remove them on unmount, even without any event prop provided (see #5058)', () => { const { navigation, - checkAddListenerCalled, - checkAddListenerCalledWith, - } = createNavigationAndHelpers(); - const eventListenerProps = createEventListenersProp(); + NavigationListenersAPI, + } = createTestNavigationAndHelpers(); + const component = renderer.create( - + ); - checkAddListenerCalled(4); - checkAddListenerCalledWith('willBlur', eventListenerProps.onWillBlur); - checkAddListenerCalledWith('willFocus', eventListenerProps.onWillFocus); - checkAddListenerCalledWith('didBlur', eventListenerProps.onDidBlur); - checkAddListenerCalledWith('didFocus', eventListenerProps.onDidFocus); + expect(NavigationListenersAPI.get('willFocus').length).toBe(1); + expect(NavigationListenersAPI.get('didFocus').length).toBe(1); + expect(NavigationListenersAPI.get('willBlur').length).toBe(1); + expect(NavigationListenersAPI.get('didBlur').length).toBe(1); + component.unmount(); + expect(NavigationListenersAPI.get('willFocus').length).toBe(0); + expect(NavigationListenersAPI.get('didFocus').length).toBe(0); + expect(NavigationListenersAPI.get('willBlur').length).toBe(0); + expect(NavigationListenersAPI.get('didBlur').length).toBe(0); }); - it('add all listeners with navigation context', () => { + it('support context-provided navigation', () => { const { navigation, - checkAddListenerCalled, - checkAddListenerCalledWith, - } = createNavigationAndHelpers(); - const eventListenerProps = createEventListenersProp(); + NavigationListenersAPI, + } = createTestNavigationAndHelpers(); const component = renderer.create( - + + + ); - checkAddListenerCalled(4); - checkAddListenerCalledWith('willBlur', eventListenerProps.onWillBlur); - checkAddListenerCalledWith('willFocus', eventListenerProps.onWillFocus); - checkAddListenerCalledWith('didBlur', eventListenerProps.onDidBlur); - checkAddListenerCalledWith('didFocus', eventListenerProps.onDidFocus); + + expect(NavigationListenersAPI.get('willFocus').length).toBe(1); + expect(NavigationListenersAPI.get('didFocus').length).toBe(1); + expect(NavigationListenersAPI.get('willBlur').length).toBe(1); + expect(NavigationListenersAPI.get('didBlur').length).toBe(1); + component.unmount(); + expect(NavigationListenersAPI.get('willFocus').length).toBe(0); + expect(NavigationListenersAPI.get('didFocus').length).toBe(0); + expect(NavigationListenersAPI.get('willBlur').length).toBe(0); + expect(NavigationListenersAPI.get('didBlur').length).toBe(0); }); - it('remove all listeners on unmount', () => { + it('wire props listeners to navigation listeners', () => { const { navigation, - checkRemoveCalled, - checkRemoveCalledWith, - } = createNavigationAndHelpers(); + NavigationListenersAPI, + } = createTestNavigationAndHelpers(); + const eventListenerProps = createEventListenersProp(); - - const component = renderer.create( - + renderer.create( + ); - checkRemoveCalled(0); - component.unmount(); - checkRemoveCalled(4); - checkRemoveCalledWith('willBlur', eventListenerProps.onWillBlur); - checkRemoveCalledWith('willFocus', eventListenerProps.onWillFocus); - checkRemoveCalledWith('didBlur', eventListenerProps.onDidBlur); - checkRemoveCalledWith('didFocus', eventListenerProps.onDidFocus); + + const checkPropListenerIsCalled = (eventName, propName) => { + expect(eventListenerProps[propName]).toHaveBeenCalledTimes(0); + NavigationListenersAPI.call(eventName); + expect(eventListenerProps[propName]).toHaveBeenCalledTimes(1); + }; + + checkPropListenerIsCalled('willFocus', 'onWillFocus'); + checkPropListenerIsCalled('didFocus', 'onDidFocus'); + checkPropListenerIsCalled('willBlur', 'onWillBlur'); + checkPropListenerIsCalled('didBlur', 'onDidBlur'); }); - it('add a single listener', () => { + it('wire latest props listener to navigation listeners on updates (support closure/arrow functions update)', () => { const { navigation, - checkAddListenerCalled, - checkAddListenerCalledWith, - } = createNavigationAndHelpers(); - const listener = createListener(); - const component = renderer.create( - - ); - checkAddListenerCalled(1); - checkAddListenerCalledWith('didFocus', listener); - component.unmount(); - }); - - it('do not attempt to add/remove stable listeners on update', () => { - const { - navigation, - checkAddListenerCalled, - checkAddListenerCalledWith, - } = createNavigationAndHelpers(); - const eventListenerProps = createEventListenersProp(); - const component = renderer.create( - - ); - component.update( - - ); - component.update( - - ); - checkAddListenerCalled(4); - checkAddListenerCalledWith('willBlur', eventListenerProps.onWillBlur); - checkAddListenerCalledWith('willFocus', eventListenerProps.onWillFocus); - checkAddListenerCalledWith('didBlur', eventListenerProps.onDidBlur); - checkAddListenerCalledWith('didFocus', eventListenerProps.onDidFocus); - }); - - it('add, remove and replace (remove+add) listeners on complex updates', () => { - const { - navigation, - checkAddListenerCalled, - checkAddListenerCalledWith, - checkRemoveCalled, - checkRemoveCalledWith, - } = createNavigationAndHelpers(); - const eventListenerProps = createEventListenersProp(); + NavigationListenersAPI, + } = createTestNavigationAndHelpers(); const component = renderer.create( - ); - checkAddListenerCalled(4); - checkAddListenerCalledWith('willBlur', eventListenerProps.onWillBlur); - checkAddListenerCalledWith('willFocus', eventListenerProps.onWillFocus); - checkAddListenerCalledWith('didBlur', eventListenerProps.onDidBlur); - checkAddListenerCalledWith('didFocus', eventListenerProps.onDidFocus); - checkRemoveCalled(0); - - const onWillFocus2 = createListener(); - const onDidFocus2 = createListener(); - component.update( - { + throw new Error('should not be called'); + }} + onDidFocus={() => { + throw new Error('should not be called'); + }} /> ); - checkAddListenerCalled(6); - checkAddListenerCalledWith('willFocus', onWillFocus2); - checkAddListenerCalledWith('didFocus', onDidFocus2); - checkRemoveCalled(3); - checkRemoveCalledWith('didBlur', eventListenerProps.onDidBlur); - checkRemoveCalledWith('willFocus', eventListenerProps.onWillFocus); - checkRemoveCalledWith('didFocus', eventListenerProps.onDidFocus); + + component.update( + + ); + + const latestEventListenerProps = createEventListenersProp(); + component.update( + + ); + + const checkLatestPropListenerCalled = (eventName, propName) => { + expect(latestEventListenerProps[propName]).toHaveBeenCalledTimes(0); + NavigationListenersAPI.call(eventName); + expect(latestEventListenerProps[propName]).toHaveBeenCalledTimes(1); + }; + + checkLatestPropListenerCalled('willFocus', 'onWillFocus'); + checkLatestPropListenerCalled('didFocus', 'onDidFocus'); + checkLatestPropListenerCalled('willBlur', 'onWillBlur'); + checkLatestPropListenerCalled('didBlur', 'onDidBlur'); }); });