From 9c11b0d4c8f9da782345865ddfb6b4adb2d7cd9b Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Mon, 26 Oct 2020 10:29:30 -0400 Subject: [PATCH 01/18] Copy additional functionality to hook --- src/__tests__/hooks.test.js | 6 +- src/useTracking.js | 129 +++++++++++++++++++++++++++++++----- 2 files changed, 117 insertions(+), 18 deletions(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 0003bf6..ef5fb23 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -13,7 +13,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('../'); @@ -416,12 +416,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}; }; diff --git a/src/useTracking.js b/src/useTracking.js index d472db8..eba74cc 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -1,25 +1,124 @@ -import { useContext, useMemo } from 'react'; +import React, { useCallback, useContext, useEffect, useMemo } from 'react'; +import merge from 'deepmerge'; + import { ReactTrackingContext } from './withTrackingComponentDecorator'; +import dispatchTrackingEvent from './dispatchTrackingEvent'; -export default function useTracking() { +export default function useTracking( + trackingData = {}, + { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} +) { const trackingContext = useContext(ReactTrackingContext); - 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`?' - ); - } + // 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`?' + // ); + // } + + // statically extract tracking.process for hook dependency + const tracking = useMemo(() => trackingContext.tracking || {}, [ + trackingContext.tracking, + ]); + const trkProcess = tracking.process; + const getProcessFn = useCallback(() => trkProcess, [trkProcess]); + + const getOwnTrackingData = useCallback(() => { + const ownTrackingData = + typeof trackingData === 'function' ? trackingData() : trackingData; + return ownTrackingData || {}; + }, [trackingData]); + + 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, + ]); + + const contextValue = useMemo( + () => ({ + tracking: { + dispatch: getTrackingDispatcher(), + getTrackingData: getTrackingDataFn(), + process: getProcessFn() || process, + }, + }), + [getTrackingDispatcher, getTrackingDataFn, getProcessFn, process] + ); + + const TrackingProvider = useCallback( + ({ children }) => ( + + {children} + + ), + [contextValue] + ); return useMemo( () => ({ - getTrackingData: trackingContext.tracking.getTrackingData, - trackEvent: trackingContext.tracking.dispatch, + getTrackingData: tracking.getTrackingData, + trackEvent: tracking.dispatch, + Track: TrackingProvider, }), - [ - trackingContext.tracking.getTrackingData, - trackingContext.tracking.dispatch, - ] + [tracking.getTrackingData, tracking.dispatch, TrackingProvider] ); } From 6303cd4ea0f8ccee8a7a040b311cebac04bea438 Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Mon, 26 Oct 2020 23:15:49 -0400 Subject: [PATCH 02/18] Call hook from decorator --- src/__tests__/e2e.test.js | 4 +- src/__tests__/hooks.test.js | 22 ++--- src/useTracking.js | 22 +++-- src/withTrackingComponentDecorator.js | 115 +++----------------------- 4 files changed, 36 insertions(+), 127 deletions(-) diff --git a/src/__tests__/e2e.test.js b/src/__tests__/e2e.test.js index 7965fc6..bcda096 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('../useTracking'); // eslint-disable-line global-require const App = track()(() => { return ; diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index ef5fb23..28029eb 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -52,7 +52,7 @@ describe('hooks', () => { const testPageData = { page: 'TestPage' }; const TestPage = () => { - const { Track } = useTracking({}, { dispatchOnMount: true }); + const { Track } = useTracking(testPageData, { dispatchOnMount: true }); return {null}; // TODO: what does it mean for the API if this was instead of useTracking({ dispatchOnMount: true }) above? }; @@ -89,13 +89,9 @@ describe('hooks', () => { const testChildData = { page: 'TestChild' }; const TestOptions = ({ children }) => { - const { Track } = useTracking(); + const { Track } = useTracking(testDataContext, { dispatch }); - return ( - - {children} - - ); + return {children}; }; const TestChild = () => { @@ -491,8 +487,8 @@ describe('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 +499,7 @@ describe('hooks', () => { }; mount( - + ); @@ -657,7 +653,7 @@ describe('hooks', () => { 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 @@ -743,9 +739,7 @@ describe('hooks', () => { }); it('root context items are accessible to children', () => { - const { - ReactTrackingContext, - } = require('../withTrackingComponentDecorator'); // eslint-disable-line global-require + const { ReactTrackingContext } = require('../useTracking'); // eslint-disable-line global-require const Child = () => { const trackingContext = useContext(ReactTrackingContext); diff --git a/src/useTracking.js b/src/useTracking.js index eba74cc..e3a278d 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -1,9 +1,10 @@ import React, { useCallback, useContext, useEffect, useMemo } from 'react'; import merge from 'deepmerge'; -import { ReactTrackingContext } from './withTrackingComponentDecorator'; import dispatchTrackingEvent from './dispatchTrackingEvent'; +export const ReactTrackingContext = React.createContext({}); + export default function useTracking( trackingData = {}, { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} @@ -93,15 +94,22 @@ export default function useTracking( process, ]); + const nextDispatch = useMemo(() => getTrackingDispatcher(), [ + getTrackingDispatcher, + ]); + const nextGetTrackingData = useMemo(() => getTrackingDataFn(), [ + getTrackingDataFn, + ]); + const contextValue = useMemo( () => ({ tracking: { - dispatch: getTrackingDispatcher(), - getTrackingData: getTrackingDataFn(), + dispatch: nextDispatch, + getTrackingData: nextGetTrackingData, process: getProcessFn() || process, }, }), - [getTrackingDispatcher, getTrackingDataFn, getProcessFn, process] + [nextDispatch, nextGetTrackingData, getProcessFn, process] ); const TrackingProvider = useCallback( @@ -115,10 +123,10 @@ export default function useTracking( return useMemo( () => ({ - getTrackingData: tracking.getTrackingData, - trackEvent: tracking.dispatch, + getTrackingData: nextGetTrackingData, + trackEvent: nextDispatch, Track: TrackingProvider, }), - [tracking.getTrackingData, tracking.dispatch, TrackingProvider] + [nextDispatch, nextGetTrackingData, TrackingProvider] ); } diff --git a/src/withTrackingComponentDecorator.js b/src/withTrackingComponentDecorator.js index 510f7d6..a38329c 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -1,33 +1,18 @@ /* eslint-disable react/jsx-props-no-spreading */ -import React, { - useCallback, - useContext, - useEffect, - useMemo, - useRef, -} from 'react'; -import merge from 'deepmerge'; +import React, { useEffect, useMemo, useRef } from 'react'; import hoistNonReactStatic from 'hoist-non-react-statics'; -import dispatchTrackingEvent from './dispatchTrackingEvent'; - -export const ReactTrackingContext = React.createContext({}); +import useTracking from './useTracking'; 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 +22,17 @@ 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 = - 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] + const { getTrackingData, trackEvent, Track } = useTracking( + trackingData, + options ); - 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 trackingProp = useMemo( () => ({ trackEvent, - getTrackingData: getTrackingDataFn(), - }), - [trackEvent, getTrackingDataFn] - ); - - const contextValue = useMemo( - () => ({ - tracking: { - dispatch: getTrackingDispatcher(), - getTrackingData: getTrackingDataFn(), - process: getProcessFn() || process, - }, + getTrackingData, }), - [getTrackingDispatcher, getTrackingDataFn, getProcessFn] + [trackEvent, getTrackingData] ); const propsToBePassed = useMemo( @@ -128,13 +40,10 @@ export default function withTrackingComponentDecorator( [props, rtFwdRef] ); - return useMemo( - () => ( - - - - ), - [contextValue, trackingProp, propsToBePassed] + return ( + + + ); } From c707421ce67b3c5fdb2609120aa6c57a224a552f Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Tue, 27 Oct 2020 22:24:16 -0400 Subject: [PATCH 03/18] Fix props issue & tests --- src/__tests__/e2e.test.js | 12 ++++++++---- src/__tests__/hooks.test.js | 6 +++--- src/withTrackingComponentDecorator.js | 12 ++++++++++-- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/__tests__/e2e.test.js b/src/__tests__/e2e.test.js index bcda096..30653e2 100644 --- a/src/__tests__/e2e.test.js +++ b/src/__tests__/e2e.test.js @@ -802,8 +802,10 @@ describe('e2e', () => { } } - // Get the first child since the page is wrapped with the WithTracking component. - const page = await mount().childAt(0); + // Get the children's children since Page is wrapped with the WithTracking & Track components. + const page = await mount() + .children() + .children(); await page.instance().executeAction(); expect(page.state().data).toEqual(message); @@ -853,8 +855,10 @@ describe('e2e', () => { } } - // Get the first child since the page is wrapped with the WithTracking component. - const page = await mount().childAt(0); + // Get the children's children since Page is wrapped with the WithTracking & Track components. + const page = await mount() + .children() + .children(); await page.instance().executeAction(); expect(page.state().data).toEqual(message); diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 28029eb..cd19b00 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -802,7 +802,7 @@ describe('hooks', () => { }); }); - it('dispatches tracking event from async function', async () => { + it.skip('dispatches tracking event from async function', async () => { const message = { value: 'test' }; let executeAction; @@ -844,7 +844,7 @@ describe('hooks', () => { }); }); - it('handles rejected async function', async () => { + it.skip('handles rejected async function', async () => { const message = { value: 'error' }; let executeAction; @@ -887,7 +887,7 @@ describe('hooks', () => { }); }); - it('can access wrapped component by ref', async () => { + it.skip('can access wrapped component by ref', async () => { const focusFn = jest.fn(); const Child = React.forwardRef((props, ref) => { diff --git a/src/withTrackingComponentDecorator.js b/src/withTrackingComponentDecorator.js index a38329c..9953432 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -1,5 +1,5 @@ /* eslint-disable react/jsx-props-no-spreading */ -import React, { useEffect, useMemo, useRef } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef } from 'react'; import hoistNonReactStatic from 'hoist-non-react-statics'; import useTracking from './useTracking'; @@ -22,8 +22,16 @@ export default function withTrackingComponentDecorator( latestProps.current = props; }); + const trackingDataFn = useCallback( + () => + typeof trackingData === 'function' + ? trackingData(latestProps.current) + : trackingData, + [] + ); + const { getTrackingData, trackEvent, Track } = useTracking( - trackingData, + trackingDataFn, options ); From c621ed8f063a2b498c71392c9e6b256d29a153eb Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Tue, 27 Oct 2020 22:32:41 -0400 Subject: [PATCH 04/18] Remove top-level hook warning --- src/useTracking.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/useTracking.js b/src/useTracking.js index e3a278d..f909157 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -11,14 +11,6 @@ export default function useTracking( ) { const trackingContext = useContext(ReactTrackingContext); - // 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`?' - // ); - // } - // statically extract tracking.process for hook dependency const tracking = useMemo(() => trackingContext.tracking || {}, [ trackingContext.tracking, From 67d18d25911c8a716a3dbe2f71ed324651be04df Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Wed, 28 Oct 2020 22:34:24 -0400 Subject: [PATCH 05/18] Unskip & update hook tests --- src/__tests__/hooks.test.js | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index cd19b00..5da89fa 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -802,8 +802,8 @@ describe('hooks', () => { }); }); - it.skip('dispatches tracking event from async function', async () => { - const message = { value: 'test' }; + it('dispatches tracking event from async function', async () => { + const message = 'test'; let executeAction; const Page = () => { @@ -829,23 +829,20 @@ describe('hooks', () => { return
{state && state.data}
; }; - // Get the first child since the page is wrapped with the WithTracking component. - const page = await mount().childAt(0); - await page.instance().executeAction(); // TODO: this probably doesn't work (how well does Enzyme support hooks?) - // TODO: maybe have to just call executeAction(); here - executeAction(); + const page = await mount(); + await executeAction(); - expect(page.state().data).toEqual(message); + expect(page.text()).toEqual(message); expect(dispatchTrackingEvent).toHaveBeenCalledTimes(1); expect(dispatchTrackingEvent).toHaveBeenCalledWith({ label: 'async action', status: 'success', - ...message, + value: message, }); }); - it.skip('handles rejected async function', async () => { - const message = { value: 'error' }; + it('handles rejected async function', async () => { + const message = 'error'; let executeAction; const Page = () => { @@ -873,13 +870,11 @@ describe('hooks', () => { return
{state && state.data}
; }; - // Get the first child since the page is wrapped with the WithTracking component. - const page = await mount().childAt(0); - await page.instance().executeAction(); + const page = await mount(); // TODO: redundant with previous test perhaps, but here for posterity. Similarly we may need to call executeAction(); directly - executeAction(); + await executeAction(); - expect(page.state().data).toEqual(message); + expect(page.text()).toEqual(message); expect(dispatchTrackingEvent).toHaveBeenCalledTimes(1); expect(dispatchTrackingEvent).toHaveBeenCalledWith({ label: 'async action', From e1ec87de667a83656c9321c28f80dba2fc0c3a84 Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Wed, 28 Oct 2020 22:58:36 -0400 Subject: [PATCH 06/18] Remove lint warnings --- src/withTrackingComponentDecorator.js | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/withTrackingComponentDecorator.js b/src/withTrackingComponentDecorator.js index 9953432..a128409 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -1,6 +1,6 @@ -/* eslint-disable react/jsx-props-no-spreading */ import React, { useCallback, useEffect, useMemo, useRef } from 'react'; import hoistNonReactStatic from 'hoist-non-react-statics'; +import PropTypes from 'prop-types'; import useTracking from './useTracking'; @@ -50,21 +50,32 @@ export default function withTrackingComponentDecorator( 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); return forwarded; } WithTracking.displayName = `WithTracking(${decoratedComponentName})`; + WithTracking.propTypes = { + rtFwdRef: PropTypes.oneOfType([ + PropTypes.func, + PropTypes.shape({ current: PropTypes.any }), + ]), + }; + WithTracking.defaultProps = { rtFwdRef: undefined }; + hoistNonReactStatic(WithTracking, DecoratedComponent); return WithTracking; }; From e9902af740ca8250986e6d93281b316c9caadf54 Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Thu, 29 Oct 2020 21:59:47 -0400 Subject: [PATCH 07/18] Remove extraneous loc --- src/useTracking.js | 9 +++------ src/withTrackingComponentDecorator.js | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/useTracking.js b/src/useTracking.js index f909157..9f9e22f 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -9,14 +9,11 @@ export default function useTracking( trackingData = {}, { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} ) { - const trackingContext = useContext(ReactTrackingContext); + const { tracking } = useContext(ReactTrackingContext); - // statically extract tracking.process for hook dependency - const tracking = useMemo(() => trackingContext.tracking || {}, [ - trackingContext.tracking, + const getProcessFn = useCallback(() => tracking && tracking.process, [ + tracking, ]); - const trkProcess = tracking.process; - const getProcessFn = useCallback(() => trkProcess, [trkProcess]); const getOwnTrackingData = useCallback(() => { const ownTrackingData = diff --git a/src/withTrackingComponentDecorator.js b/src/withTrackingComponentDecorator.js index a128409..57f2104 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -1,5 +1,5 @@ import React, { useCallback, useEffect, useMemo, useRef } from 'react'; -import hoistNonReactStatic from 'hoist-non-react-statics'; +import hoistNonReactStatics from 'hoist-non-react-statics'; import PropTypes from 'prop-types'; import useTracking from './useTracking'; @@ -63,7 +63,7 @@ export default function withTrackingComponentDecorator( React.createElement(WithTracking, { ...props, rtFwdRef: ref }) ); forwarded.displayName = `WithTracking(${decoratedComponentName})`; - hoistNonReactStatic(forwarded, DecoratedComponent); + hoistNonReactStatics(forwarded, DecoratedComponent); return forwarded; } @@ -76,7 +76,7 @@ export default function withTrackingComponentDecorator( }; WithTracking.defaultProps = { rtFwdRef: undefined }; - hoistNonReactStatic(WithTracking, DecoratedComponent); + hoistNonReactStatics(WithTracking, DecoratedComponent); return WithTracking; }; } From c032079eb34b5f84594972dd27e5765096e6ab4b Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Fri, 30 Oct 2020 21:43:12 -0400 Subject: [PATCH 08/18] Move core functionality into separate hook --- src/__tests__/e2e.test.js | 14 ++-- src/__tests__/hooks.test.js | 32 +------- src/useTracking.js | 114 ++------------------------ src/useTrackingImpl.js | 96 ++++++++++++++++++++++ src/withTrackingComponentDecorator.js | 17 ++-- 5 files changed, 118 insertions(+), 155 deletions(-) create mode 100644 src/useTrackingImpl.js diff --git a/src/__tests__/e2e.test.js b/src/__tests__/e2e.test.js index 30653e2..c876706 100644 --- a/src/__tests__/e2e.test.js +++ b/src/__tests__/e2e.test.js @@ -711,7 +711,7 @@ describe('e2e', () => { }); it('root context items are accessible to children', () => { - const { ReactTrackingContext } = require('../useTracking'); // eslint-disable-line global-require + const { ReactTrackingContext } = require('../useTrackingImpl'); // eslint-disable-line global-require const App = track()(() => { return ; @@ -802,10 +802,8 @@ describe('e2e', () => { } } - // Get the children's children since Page is wrapped with the WithTracking & Track components. - const page = await mount() - .children() - .children(); + // Get the first child since the page is wrapped with the WithTracking component. + const page = await mount().childAt(0); await page.instance().executeAction(); expect(page.state().data).toEqual(message); @@ -855,10 +853,8 @@ describe('e2e', () => { } } - // Get the children's children since Page is wrapped with the WithTracking & Track components. - const page = await mount() - .children() - .children(); + // Get the first child since the page is wrapped with the WithTracking component. + const page = await mount().childAt(0); await page.instance().executeAction(); expect(page.state().data).toEqual(message); diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 5da89fa..7a83ae5 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -739,7 +739,7 @@ describe('hooks', () => { }); it('root context items are accessible to children', () => { - const { ReactTrackingContext } = require('../useTracking'); // eslint-disable-line global-require + const { ReactTrackingContext } = require('../useTrackingImpl'); // eslint-disable-line global-require const Child = () => { const trackingContext = useContext(ReactTrackingContext); @@ -881,34 +881,4 @@ describe('hooks', () => { status: 'failed', }); }); - - it.skip('can access wrapped component by ref', async () => { - const focusFn = jest.fn(); - - const Child = React.forwardRef((props, ref) => { - const { Track } = useTracking({}); - - 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/useTracking.js b/src/useTracking.js index 9f9e22f..76b4331 100644 --- a/src/useTracking.js +++ b/src/useTracking.js @@ -1,107 +1,11 @@ -import React, { useCallback, useContext, useEffect, useMemo } from 'react'; -import merge from 'deepmerge'; +import React, { useCallback, useMemo } from 'react'; -import dispatchTrackingEvent from './dispatchTrackingEvent'; +import useTrackingImpl, { ReactTrackingContext } from './useTrackingImpl'; -export const ReactTrackingContext = React.createContext({}); +export default function useTracking(trackingData, options) { + const contextValue = useTrackingImpl(trackingData, options); -export default function useTracking( - trackingData = {}, - { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} -) { - const { tracking } = useContext(ReactTrackingContext); - - const getProcessFn = useCallback(() => tracking && tracking.process, [ - tracking, - ]); - - const getOwnTrackingData = useCallback(() => { - const ownTrackingData = - typeof trackingData === 'function' ? trackingData() : trackingData; - return ownTrackingData || {}; - }, [trackingData]); - - 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, - ]); - - const nextDispatch = useMemo(() => getTrackingDispatcher(), [ - getTrackingDispatcher, - ]); - const nextGetTrackingData = useMemo(() => getTrackingDataFn(), [ - getTrackingDataFn, - ]); - - const contextValue = useMemo( - () => ({ - tracking: { - dispatch: nextDispatch, - getTrackingData: nextGetTrackingData, - process: getProcessFn() || process, - }, - }), - [nextDispatch, nextGetTrackingData, getProcessFn, process] - ); - - const TrackingProvider = useCallback( + const Track = useCallback( ({ children }) => ( {children} @@ -112,10 +16,10 @@ export default function useTracking( return useMemo( () => ({ - getTrackingData: nextGetTrackingData, - trackEvent: nextDispatch, - Track: TrackingProvider, + Track, + getTrackingData: contextValue.tracking.getTrackingData, + trackEvent: contextValue.tracking.dispatch, }), - [nextDispatch, nextGetTrackingData, TrackingProvider] + [contextValue, Track] ); } diff --git a/src/useTrackingImpl.js b/src/useTrackingImpl.js new file mode 100644 index 0000000..c045dc7 --- /dev/null +++ b/src/useTrackingImpl.js @@ -0,0 +1,96 @@ +import React, { useCallback, useContext, useEffect, useMemo } from 'react'; +import merge from 'deepmerge'; + +import dispatchTrackingEvent from './dispatchTrackingEvent'; + +export const ReactTrackingContext = React.createContext({}); + +export default function useTrackingImpl( + trackingData = {}, + { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} +) { + const { tracking } = useContext(ReactTrackingContext); + + const getProcessFn = useCallback(() => tracking && tracking.process, [ + tracking, + ]); + + const getOwnTrackingData = useCallback(() => { + const ownTrackingData = + typeof trackingData === 'function' ? trackingData() : trackingData; + return ownTrackingData || {}; + }, [trackingData]); + + 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 57f2104..7f39c21 100644 --- a/src/withTrackingComponentDecorator.js +++ b/src/withTrackingComponentDecorator.js @@ -2,7 +2,7 @@ import React, { useCallback, useEffect, useMemo, useRef } from 'react'; import hoistNonReactStatics from 'hoist-non-react-statics'; import PropTypes from 'prop-types'; -import useTracking from './useTracking'; +import useTrackingImpl, { ReactTrackingContext } from './useTrackingImpl'; export default function withTrackingComponentDecorator( trackingData = {}, @@ -30,17 +30,14 @@ export default function withTrackingComponentDecorator( [] ); - const { getTrackingData, trackEvent, Track } = useTracking( - trackingDataFn, - options - ); + const contextValue = useTrackingImpl(trackingDataFn, options); const trackingProp = useMemo( () => ({ - trackEvent, - getTrackingData, + trackEvent: contextValue.tracking.dispatch, + getTrackingData: contextValue.tracking.getTrackingData, }), - [trackEvent, getTrackingData] + [contextValue] ); const propsToBePassed = useMemo( @@ -49,12 +46,12 @@ export default function withTrackingComponentDecorator( ); return ( - + {React.createElement(DecoratedComponent, { ...propsToBePassed, tracking: trackingProp, })} - + ); } From 584188a699c05f13adc2c31277b8b42e7e90cfb7 Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Fri, 30 Oct 2020 22:17:05 -0400 Subject: [PATCH 09/18] Clean up tests --- src/__tests__/hooks.test.js | 39 ++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 7a83ae5..5c41e8f 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -54,7 +54,7 @@ describe('hooks', () => { const TestPage = () => { const { Track } = useTracking(testPageData, { dispatchOnMount: true }); - return {null}; // TODO: what does it mean for the API if this was instead of useTracking({ dispatchOnMount: true }) above? + return {null}; }; mount(); @@ -66,7 +66,7 @@ describe('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); @@ -95,7 +95,7 @@ describe('hooks', () => { }; 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
; }; @@ -163,7 +163,7 @@ describe('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; }; @@ -192,7 +192,7 @@ describe('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
; }; @@ -210,7 +210,6 @@ describe('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 }, @@ -376,7 +375,7 @@ describe('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
; }; @@ -805,7 +804,6 @@ describe('hooks', () => { it('dispatches tracking event from async function', async () => { const message = 'test'; - let executeAction; const Page = () => { const [state, setState] = useState({}); const { trackEvent } = useTracking(); @@ -821,16 +819,22 @@ describe('hooks', () => { return value; }; - executeAction = async () => { + const executeAction = async () => { const data = await handleAsyncAction(); setState({ data }); }; - return
{state && state.data}
; + return ( + <> + +
+ ); + }; + + const wrapper = mount(); + + wrapper.find('button').simulate('click'); + + expect(dispatchCount).toEqual(1); + }); + + 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(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' }; @@ -702,7 +774,7 @@ describe('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' }; diff --git a/src/useTrackingImpl.js b/src/useTrackingImpl.js index d02abd9..4144dbb 100644 --- a/src/useTrackingImpl.js +++ b/src/useTrackingImpl.js @@ -1,24 +1,34 @@ -import { useCallback, useContext, useEffect, useMemo } from 'react'; +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 = {}, - { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = {} -) { +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 } = + latestOptions.current || {}; const getProcessFn = useCallback(() => tracking && tracking.process, [ tracking, ]); const getOwnTrackingData = useCallback(() => { - const ownTrackingData = - typeof trackingData === 'function' ? trackingData() : trackingData; + const data = latestData.current; + const ownTrackingData = typeof data === 'function' ? data() : data; return ownTrackingData || {}; - }, [trackingData]); + }, []); const getTrackingDataFn = useCallback(() => { const contextGetTrackingData = From 765276c6e050f127d7bfcdba9296a1c6a13d90fd Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Thu, 5 Nov 2020 22:35:37 -0500 Subject: [PATCH 16/18] Update hooks test to call dispatch mock Co-authored-by: Jeremy Gayed --- src/__tests__/hooks.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index f76a344..69e4568 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -663,8 +663,9 @@ describe('hooks', () => { useTracking( {}, { + dispatch, dispatchOnMount: () => { - dispatchCount += 1; + return { test: true }; }, } ); From 5d04a23c206e363a3168b8b2d1a4b3841de5e64f Mon Sep 17 00:00:00 2001 From: Bryan Gergen Date: Thu, 5 Nov 2020 23:53:51 -0500 Subject: [PATCH 17/18] Update tests, fix memoization bug --- src/__tests__/hooks.test.js | 35 +++++++++++++++++++++-------------- src/useTrackingImpl.js | 7 +++++-- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index 69e4568..c8424c1 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -627,18 +627,15 @@ describe('hooks', () => { }); const App = track()(() => { - const [state, setState] = React.useState({}); + const [count, setCount] = useState(0); return (

Extra renders of InnerComponent caused by new context API

- - + @@ -655,10 +652,16 @@ describe('hooks', () => { }); it('does not cause unnecessary dispatches due to object literals passed to useTracking', () => { - let dispatchCount = 0; + const trackRenders = jest.fn(); const App = () => { - const [state, setState] = React.useState({}); + // 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( {}, @@ -675,10 +678,7 @@ describe('hooks', () => {

Extra dispatches caused by new object literals passed on re-render

-
@@ -687,9 +687,16 @@ describe('hooks', () => { const wrapper = mount(); - wrapper.find('button').simulate('click'); + const button = wrapper.find('button'); + button.simulate('click'); + button.simulate('click'); + button.simulate('click'); - expect(dispatchCount).toEqual(1); + expect(trackRenders).toHaveBeenCalledTimes(4); + expect(dispatch).toHaveBeenCalledTimes(1); + expect(dispatch).toHaveBeenLastCalledWith({ + test: true, + }); }); it('dispatches the correct data if props change', () => { diff --git a/src/useTrackingImpl.js b/src/useTrackingImpl.js index 4144dbb..5e9432c 100644 --- a/src/useTrackingImpl.js +++ b/src/useTrackingImpl.js @@ -17,8 +17,11 @@ export default function useTrackingImpl(trackingData, options) { latestOptions.current = options; }); - const { dispatch = dispatchTrackingEvent, dispatchOnMount = false, process } = - latestOptions.current || {}; + const { + dispatch = dispatchTrackingEvent, + dispatchOnMount = false, + process, + } = useMemo(() => latestOptions.current || {}, []); const getProcessFn = useCallback(() => tracking && tracking.process, [ tracking, From 754785568b0ade930131b3191dbe48541b0c111a Mon Sep 17 00:00:00 2001 From: Jeremy Gayed Date: Sun, 8 Nov 2020 16:22:06 -0500 Subject: [PATCH 18/18] tweak + small new test (#170) --- src/__tests__/hooks.test.js | 83 +++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 22 deletions(-) diff --git a/src/__tests__/hooks.test.js b/src/__tests__/hooks.test.js index c8424c1..071987d 100644 --- a/src/__tests__/hooks.test.js +++ b/src/__tests__/hooks.test.js @@ -609,44 +609,83 @@ describe('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; - }); - const App = track()(() => { + // 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

- - - - - - -
+ +
+

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', + }); expect(innerRenderCount).toEqual(1); });