diff --git a/src/ReactTrackingContext.js b/src/ReactTrackingContext.js new file mode 100644 index 0000000..166e08b --- /dev/null +++ b/src/ReactTrackingContext.js @@ -0,0 +1,3 @@ +import React from 'react'; + +export default React.createContext({}); diff --git a/src/__tests__/e2e.test.js b/src/__tests__/e2e.test.js index 7965fc6..510b7c8 100644 --- a/src/__tests__/e2e.test.js +++ b/src/__tests__/e2e.test.js @@ -711,9 +711,7 @@ describe('e2e', () => { }); it('root context items are accessible to children', () => { - const { - ReactTrackingContext, - } = require('../withTrackingComponentDecorator'); // eslint-disable-line global-require + const ReactTrackingContext = require('../ReactTrackingContext').default; // eslint-disable-line global-require const App = track()(() => { return ; diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 0003bf6..071987d 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -3,6 +3,7 @@ /* eslint-disable react/destructuring-assignment,react/no-multi-comp,react/prop-types,react/prefer-stateless-function */ /* eslint-disable jsx-a11y/control-has-associated-label */ import React, { useContext, useEffect, useState } from 'react'; +import { act } from 'react-dom/test-utils'; import { mount } from 'enzyme'; const dispatchTrackingEvent = jest.fn(); @@ -13,7 +14,7 @@ const testData = { testData: true }; const dispatch = jest.fn(); const testState = { booleanState: true }; -describe.skip('hooks', () => { +describe('hooks', () => { // eslint-disable-next-line global-require const { default: track, useTracking } = require('../'); @@ -52,9 +53,8 @@ describe.skip('hooks', () => { const testPageData = { page: 'TestPage' }; const TestPage = () => { - const { Track } = useTracking({}, { dispatchOnMount: true }); - - return {null}; // TODO: what does it mean for the API if this was instead of useTracking({ dispatchOnMount: true }) above? + useTracking(testPageData, { dispatchOnMount: true }); + return null; }; mount(); @@ -66,7 +66,7 @@ describe.skip('hooks', () => { it('accepts a dispatch function in options', () => { const TestOptions = () => { - const { trackEvent } = useTracking(testDataContext, { dispatch }); // TODO: should we accept top-level config options here? + const { trackEvent } = useTracking(testDataContext, { dispatch }); const blah = () => { trackEvent(testData); @@ -89,17 +89,13 @@ describe.skip('hooks', () => { const testChildData = { page: 'TestChild' }; const TestOptions = ({ children }) => { - const { Track } = useTracking(); + const { Track } = useTracking(testDataContext, { dispatch }); - return ( - - {children} - - ); + return {children}; }; const TestChild = () => { - useTracking(testChildData, { dispatchOnMount: true }); // TODO: Should these properties instead be on the component and we require the user to "render" it? + useTracking(testChildData, { dispatchOnMount: true }); return
; }; @@ -167,7 +163,7 @@ describe.skip('hooks', () => { const dispatchOnMount = jest.fn(() => ({ dom: true })); const TestComponent = () => { - useTracking(testDispatchOnMount, { dispatch, dispatchOnMount }); // TODO: potential recipe here is if a component does not render children, it doesn't need to include in the tree! + useTracking(testDispatchOnMount, { dispatch, dispatchOnMount }); return null; }; @@ -196,7 +192,7 @@ describe.skip('hooks', () => { }; const Page = () => { - useTracking({ page: 'Page' }); // TODO: Does this pass? It doesn't render children so we should still get all the proper tracking context dispatched on mount according to "process" fn above + useTracking({ page: 'Page' }); return
Page
; }; @@ -214,7 +210,6 @@ describe.skip('hooks', () => { }); it('will dispatch a pageview event on mount on functional component', () => { - // TODO: Using purely hooks API, this test is redundant with above, but we will keep it for parity const App = ({ children }) => { const { Track } = useTracking( { topLevel: true }, @@ -380,7 +375,7 @@ describe.skip('hooks', () => { }; const Page = ({ runtimeData }) => { - useTracking({ page: 'Page', runtimeData }); // TODO: in this case we were able to derive props.runtimeData "statically" from within the component, does this still work as expected? + useTracking({ page: 'Page', runtimeData }); return
Page
; }; @@ -416,12 +411,12 @@ describe.skip('hooks', () => { return {children}; }; const Page = ({ children }) => { - const Track = useTracking({ page: 'Page' }); + const { Track } = useTracking({ page: 'Page' }); return {children}; }; const Nested = ({ children }) => { - const Track = useTracking({ view: 'View' }); + const { Track } = useTracking({ view: 'View' }); return {children}; }; @@ -491,8 +486,8 @@ describe.skip('hooks', () => { it('can read tracking data from props.tracking.getTrackingData()', () => { const mockReader = jest.fn(); - const TestOptions = ({ onProps, children }) => { - const { Track } = useTracking({ onProps, ...testDataContext }); + const TestOptions = ({ onProps, child, children }) => { + const { Track } = useTracking({ onProps, child, ...testDataContext }); return {children}; }; @@ -503,7 +498,7 @@ describe.skip('hooks', () => { }; mount( - + ); @@ -519,7 +514,9 @@ describe.skip('hooks', () => { }); it('logs a console error when there is already a process defined on context', () => { - global.console.error = jest.fn(); + const consoleError = jest + .spyOn(global.console, 'error') + .mockImplementation(() => {}); const process = () => {}; const NestedComponent = () => { @@ -550,10 +547,12 @@ describe.skip('hooks', () => { mount(); - expect(global.console.error).toHaveBeenCalledTimes(1); - expect(global.console.error).toHaveBeenCalledWith( + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError).toHaveBeenCalledWith( '[react-tracking] options.process should be defined once on a top-level component' ); + + consoleError.mockRestore(); }); it('will dispatch different data if props changed', () => { @@ -610,54 +609,173 @@ describe.skip('hooks', () => { }); }); + it('provides passed in tracking data immediately', () => { + const Foo = () => { + const { getTrackingData } = useTracking({ seeMe: true }); + + expect(getTrackingData()).toStrictEqual({ seeMe: true }); + + return null; + }; + + mount(); + }); + it('does not cause unnecessary updates due to context changes', () => { let innerRenderCount = 0; + let getLatestTrackingData; - const OuterComponent = track()(props => props.children); + const OuterComponent = ({ children, trackedProp }) => { + const { Track } = useTracking({ trackedProp }); + return {children}; + }; - const MiddleComponent = track()( - React.memo( - props => props.children, - (props, prevProps) => props.middleProp === prevProps.middleProp - ) + const MiddleComponent = React.memo( + ({ children, middleProp }) => { + const { Track } = useTracking({ middleProp }); + return {children}; + }, + (props, prevProps) => props.middleProp === prevProps.middleProp ); - const InnerComponent = track()(() => { + const InnerComponent = ({ innerProps }) => { + const { Track, getTrackingData } = useTracking({ innerProps }); innerRenderCount += 1; - return null; + + // expose for test assertion later + getLatestTrackingData = getTrackingData; + + return {innerProps}; + }; + + const App = () => { + const [count, setCount] = useState(0); + + const { Track } = useTracking({ count }); + + return ( + +
+

Extra renders of InnerComponent caused by new context API

+ + + + + + +
+ + ); + }; + + const wrapper = mount(); + + wrapper.find('button').simulate('click'); + wrapper.find('button').simulate('click'); + + expect(getLatestTrackingData()).toStrictEqual({ + count: 2, + trackedProp: 2, + middleProp: 'middleProp', + innerProps: 'a', }); - const App = track()(() => { - const [state, setState] = React.useState({}); + expect(innerRenderCount).toEqual(1); + }); + + it('does not cause unnecessary dispatches due to object literals passed to useTracking', () => { + const trackRenders = jest.fn(); + + const App = () => { + // eslint-disable-next-line no-unused-vars + const [clickCount, setClickCount] = useState(0); + + useEffect(() => { + // use mock function to ensure that we are counting renders, not clicks + trackRenders(); + }); + + useTracking( + {}, + { + dispatch, + dispatchOnMount: () => { + return { test: true }; + }, + } + ); return (
-

Extra renders of InnerComponent caused by new context API

- - - - - -
); + }; + + const wrapper = mount(); + + const button = wrapper.find('button'); + button.simulate('click'); + button.simulate('click'); + button.simulate('click'); + + expect(trackRenders).toHaveBeenCalledTimes(4); + expect(dispatch).toHaveBeenCalledTimes(1); + expect(dispatch).toHaveBeenLastCalledWith({ + test: true, }); + }); + + it('dispatches the correct data if props change', () => { + const App = props => { + const { trackEvent } = useTracking( + { data: props.data || '' }, + { + dispatch, + dispatchOnMount: true, + } + ); + + const handleClick = () => { + trackEvent({ event: 'click' }); + }; + + return ( +
+
+ ); + }; const wrapper = mount(); + expect(dispatch).toHaveBeenCalledWith({ data: '' }); + + wrapper.setProps({ data: 'Updated data prop' }); wrapper.find('button').simulate('click'); - expect(innerRenderCount).toEqual(1); + expect(dispatch).toHaveBeenCalledTimes(2); + expect(dispatch).toHaveBeenLastCalledWith({ + event: 'click', + data: 'Updated data prop', + }); }); it('can interop with the HoC (where HoC is top-level)', () => { const mockDispatchTrackingEvent = jest.fn(); - const testData1 = { key: { x: 1, y: 1, topLevel: 'hoc' } }; + const testData1 = { key: { x: 1, y: 1 }, topLevel: 'hoc' }; const testData2 = { key: { x: 2, z: 2 }, page: 'TestDeepMerge' }; // functional wrapper hoc @@ -703,7 +821,7 @@ describe.skip('hooks', () => { }); }); - it('can interop with HoC (where Hook is top-level', () => { + it('can interop with HoC (where Hook is top-level)', () => { const mockDispatchTrackingEvent = jest.fn(); const testData1 = { key: { x: 1, y: 1 }, topLevel: 'hook' }; const testData2 = { key: { x: 2, z: 2 }, page: 'TestDeepMerge' }; @@ -743,9 +861,7 @@ describe.skip('hooks', () => { }); it('root context items are accessible to children', () => { - const { - ReactTrackingContext, - } = require('../withTrackingComponentDecorator'); // eslint-disable-line global-require + const ReactTrackingContext = require('../ReactTrackingContext').default; // eslint-disable-line global-require const Child = () => { const trackingContext = useContext(ReactTrackingContext); @@ -809,9 +925,8 @@ describe.skip('hooks', () => { }); it('dispatches tracking event from async function', async () => { - const message = { value: 'test' }; + const message = 'test'; - let executeAction; const Page = () => { const [state, setState] = useState({}); const { trackEvent } = useTracking(); @@ -827,33 +942,36 @@ describe.skip('hooks', () => { return value; }; - executeAction = async () => { + const executeAction = async () => { const data = await handleAsyncAction(); setState({ data }); }; - return
{state && state.data}
; + return ( + <> + - - ); - }); - - const ref = React.createRef(); - const Parent = () => { - useEffect(() => { - ref.current.focus(); - }, []); - - return ; - }; - - const parent = await mount(); - - expect(parent.instance().child).not.toBeNull(); // TODO: probably need to rethink how this test works - expect(focusFn).toHaveBeenCalledTimes(1); - }); }); diff --git a/src/__tests__/useTracking.test.js b/src/__tests__/useTracking.test.js index c9907a8..7a381e3 100644 --- a/src/__tests__/useTracking.test.js +++ b/src/__tests__/useTracking.test.js @@ -2,22 +2,22 @@ import { mount } from 'enzyme'; import React from 'react'; import { renderToString } from 'react-dom/server'; -import track from '../withTrackingComponentDecorator'; import useTracking from '../useTracking'; describe('useTracking', () => { - it('throws error if tracking context not present', () => { + it('does not throw an error if tracking context not present', () => { const ThrowMissingContext = () => { useTracking(); return
hi
; }; - try { - renderToString(); - } catch (error) { - expect(error.message).toContain( - 'Attempting to call `useTracking` without a ReactTrackingContext present' - ); - } + + expect(() => { + try { + renderToString(); + } catch (error) { + throw new Error(error); + } + }).not.toThrow(); }); it('dispatches tracking events from a useTracking hook tracking object', () => { @@ -27,8 +27,8 @@ describe('useTracking', () => { const dispatch = jest.fn(); - const App = track(outerTrackingData, { dispatch })(() => { - const tracking = useTracking(); + const App = () => { + const tracking = useTracking(outerTrackingData, { dispatch }); expect(tracking.getTrackingData()).toEqual({ page: 'Page', @@ -44,7 +44,7 @@ describe('useTracking', () => { } /> ); - }); + }; const wrapper = mount(); wrapper.simulate('click'); diff --git a/src/useTracking.js b/src/useTracking.js index d472db8..7e310e2 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -1,25 +1,26 @@ -import { useContext, useMemo } from 'react'; -import { ReactTrackingContext } from './withTrackingComponentDecorator'; +import React, { useCallback, useMemo } from 'react'; -export default function useTracking() { - const trackingContext = useContext(ReactTrackingContext); +import ReactTrackingContext from './ReactTrackingContext'; +import useTrackingImpl from './useTrackingImpl'; - if (!(trackingContext && trackingContext.tracking)) { - throw new Error( - 'Attempting to call `useTracking` ' + - 'without a ReactTrackingContext present. Did you forget to wrap the top of ' + - 'your component tree with `track`?' - ); - } +export default function useTracking(trackingData, options) { + const contextValue = useTrackingImpl(trackingData, options); + + const Track = useCallback( + ({ children }) => ( + + {children} + + ), + [contextValue] + ); return useMemo( () => ({ - getTrackingData: trackingContext.tracking.getTrackingData, - trackEvent: trackingContext.tracking.dispatch, + Track, + getTrackingData: contextValue.tracking.getTrackingData, + trackEvent: contextValue.tracking.dispatch, }), - [ - trackingContext.tracking.getTrackingData, - trackingContext.tracking.dispatch, - ] + [contextValue, Track] ); } diff --git a/src/useTrackingImpl.js b/src/useTrackingImpl.js new file mode 100644 index 0000000..5e9432c --- /dev/null +++ b/src/useTrackingImpl.js @@ -0,0 +1,108 @@ +import { useCallback, useContext, useEffect, useMemo, useRef } from 'react'; +import merge from 'deepmerge'; + +import ReactTrackingContext from './ReactTrackingContext'; +import dispatchTrackingEvent from './dispatchTrackingEvent'; + +export default function useTrackingImpl(trackingData, options) { + const { tracking } = useContext(ReactTrackingContext); + const latestData = useRef(trackingData); + const latestOptions = useRef(options); + + useEffect(() => { + // store the latest data & options in a mutable ref to prevent + // dependencies from changing when the consumer passes in non-memoized objects + // same approach that we use for props in withTrackingComponentDecorator + latestData.current = trackingData; + latestOptions.current = options; + }); + + const { + dispatch = dispatchTrackingEvent, + dispatchOnMount = false, + process, + } = useMemo(() => latestOptions.current || {}, []); + + const getProcessFn = useCallback(() => tracking && tracking.process, [ + tracking, + ]); + + const getOwnTrackingData = useCallback(() => { + const data = latestData.current; + const ownTrackingData = typeof data === 'function' ? data() : data; + return ownTrackingData || {}; + }, []); + + const getTrackingDataFn = useCallback(() => { + const contextGetTrackingData = + (tracking && tracking.getTrackingData) || getOwnTrackingData; + + return () => + contextGetTrackingData === getOwnTrackingData + ? getOwnTrackingData() + : merge(contextGetTrackingData(), getOwnTrackingData()); + }, [getOwnTrackingData, tracking]); + + const getTrackingDispatcher = useCallback(() => { + const contextDispatch = (tracking && tracking.dispatch) || dispatch; + return data => contextDispatch(merge(getOwnTrackingData(), data || {})); + }, [getOwnTrackingData, tracking, dispatch]); + + const trackEvent = useCallback( + (data = {}) => { + getTrackingDispatcher()(data); + }, + [getTrackingDispatcher] + ); + + useEffect(() => { + const contextProcess = getProcessFn(); + const getTrackingData = getTrackingDataFn(); + + if (contextProcess && process) { + // eslint-disable-next-line + console.error( + '[react-tracking] options.process should be defined once on a top-level component' + ); + } + + if ( + typeof contextProcess === 'function' && + typeof dispatchOnMount === 'function' + ) { + trackEvent( + merge( + contextProcess(getOwnTrackingData()) || {}, + dispatchOnMount(getTrackingData()) || {} + ) + ); + } else if (typeof contextProcess === 'function') { + const processed = contextProcess(getOwnTrackingData()); + if (processed || dispatchOnMount === true) { + trackEvent(processed); + } + } else if (typeof dispatchOnMount === 'function') { + trackEvent(dispatchOnMount(getTrackingData())); + } else if (dispatchOnMount === true) { + trackEvent(); + } + }, [ + getOwnTrackingData, + getProcessFn, + getTrackingDataFn, + trackEvent, + dispatchOnMount, + process, + ]); + + return useMemo( + () => ({ + tracking: { + dispatch: getTrackingDispatcher(), + getTrackingData: getTrackingDataFn(), + process: getProcessFn() || process, + }, + }), + [getTrackingDispatcher, getTrackingDataFn, getProcessFn, process] + ); +} diff --git a/src/withTrackingComponentDecorator.js b/src/withTrackingComponentDecorator.js index 510f7d6..bddb9a4 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -1,33 +1,19 @@ -/* eslint-disable react/jsx-props-no-spreading */ -import React, { - useCallback, - useContext, - useEffect, - useMemo, - useRef, -} from 'react'; -import merge from 'deepmerge'; -import hoistNonReactStatic from 'hoist-non-react-statics'; +import React, { useCallback, useEffect, useMemo, useRef } from 'react'; +import hoistNonReactStatics from 'hoist-non-react-statics'; +import PropTypes from 'prop-types'; -import dispatchTrackingEvent from './dispatchTrackingEvent'; - -export const ReactTrackingContext = React.createContext({}); +import ReactTrackingContext from './ReactTrackingContext'; +import useTrackingImpl from './useTrackingImpl'; export default function withTrackingComponentDecorator( trackingData = {}, - { - dispatch = dispatchTrackingEvent, - dispatchOnMount = false, - process, - forwardRef = false, - } = {} + { forwardRef = false, ...options } = {} ) { return DecoratedComponent => { const decoratedComponentName = DecoratedComponent.displayName || DecoratedComponent.name || 'Component'; function WithTracking({ rtFwdRef, ...props }) { - const { tracking } = useContext(ReactTrackingContext); const latestProps = useRef(props); useEffect(() => { @@ -37,90 +23,22 @@ export default function withTrackingComponentDecorator( latestProps.current = props; }); - // statically extract tracking.process for hook dependency - const trkProcess = tracking && tracking.process; - const getProcessFn = useCallback(() => trkProcess, [trkProcess]); - - const getOwnTrackingData = useCallback(() => { - const ownTrackingData = + const trackingDataFn = useCallback( + () => typeof trackingData === 'function' ? trackingData(latestProps.current) - : trackingData; - return ownTrackingData || {}; - }, []); - - const getTrackingDataFn = useCallback(() => { - const contextGetTrackingData = - (tracking && tracking.getTrackingData) || getOwnTrackingData; - - return () => - contextGetTrackingData === getOwnTrackingData - ? getOwnTrackingData() - : merge(contextGetTrackingData(), getOwnTrackingData()); - }, [getOwnTrackingData, tracking]); - - const getTrackingDispatcher = useCallback(() => { - const contextDispatch = (tracking && tracking.dispatch) || dispatch; - return data => contextDispatch(merge(getOwnTrackingData(), data || {})); - }, [getOwnTrackingData, tracking]); - - const trackEvent = useCallback( - (data = {}) => { - getTrackingDispatcher()(data); - }, - [getTrackingDispatcher] + : trackingData, + [] ); - useEffect(() => { - const contextProcess = getProcessFn(); - const getTrackingData = getTrackingDataFn(); - - if (getProcessFn() && process) { - // eslint-disable-next-line - console.error( - '[react-tracking] options.process should be defined once on a top-level component' - ); - } - - if ( - typeof contextProcess === 'function' && - typeof dispatchOnMount === 'function' - ) { - trackEvent( - merge( - contextProcess(getOwnTrackingData()) || {}, - dispatchOnMount(getTrackingData()) || {} - ) - ); - } else if (typeof contextProcess === 'function') { - const processed = contextProcess(getOwnTrackingData()); - if (processed || dispatchOnMount === true) { - trackEvent(processed); - } - } else if (typeof dispatchOnMount === 'function') { - trackEvent(dispatchOnMount(getTrackingData())); - } else if (dispatchOnMount === true) { - trackEvent(); - } - }, [getOwnTrackingData, getProcessFn, getTrackingDataFn, trackEvent]); + const contextValue = useTrackingImpl(trackingDataFn, options); const trackingProp = useMemo( () => ({ - trackEvent, - getTrackingData: getTrackingDataFn(), + trackEvent: contextValue.tracking.dispatch, + getTrackingData: contextValue.tracking.getTrackingData, }), - [trackEvent, getTrackingDataFn] - ); - - const contextValue = useMemo( - () => ({ - tracking: { - dispatch: getTrackingDispatcher(), - getTrackingData: getTrackingDataFn(), - process: getProcessFn() || process, - }, - }), - [getTrackingDispatcher, getTrackingDataFn, getProcessFn] + [contextValue] ); const propsToBePassed = useMemo( @@ -128,27 +46,35 @@ export default function withTrackingComponentDecorator( [props, rtFwdRef] ); - return useMemo( - () => ( - - - - ), - [contextValue, trackingProp, propsToBePassed] + return ( + + {React.createElement(DecoratedComponent, { + ...propsToBePassed, + tracking: trackingProp, + })} + ); } if (forwardRef) { - const forwarded = React.forwardRef((props, ref) => ( - - )); + const forwarded = React.forwardRef((props, ref) => + React.createElement(WithTracking, { ...props, rtFwdRef: ref }) + ); forwarded.displayName = `WithTracking(${decoratedComponentName})`; - hoistNonReactStatic(forwarded, DecoratedComponent); + hoistNonReactStatics(forwarded, DecoratedComponent); return forwarded; } WithTracking.displayName = `WithTracking(${decoratedComponentName})`; - hoistNonReactStatic(WithTracking, DecoratedComponent); + WithTracking.propTypes = { + rtFwdRef: PropTypes.oneOfType([ + PropTypes.func, + PropTypes.shape({ current: PropTypes.any }), + ]), + }; + WithTracking.defaultProps = { rtFwdRef: undefined }; + + hoistNonReactStatics(WithTracking, DecoratedComponent); return WithTracking; }; }