From 67c06face6b037d7e57e03d9d532c6c7ccc3f409 Mon Sep 17 00:00:00 2001 From: guptbhum Date: Fri, 10 Jan 2025 00:20:37 -0800 Subject: [PATCH] fix: Emit performance marks only for loaded components (#3175) Co-authored-by: Abdallah AlHalees --- pages/button/performance-marks.page.tsx | 3 +- .../__integ__/performance-marks.test.ts | 35 +++++----- src/button/internal.tsx | 2 +- .../__tests__/use-performance-marks.test.tsx | 8 ++- .../hooks/use-performance-marks/index.ts | 6 +- src/table/__integ__/performance-marks.test.ts | 64 ++++--------------- src/table/internal.tsx | 2 +- 7 files changed, 45 insertions(+), 75 deletions(-) diff --git a/pages/button/performance-marks.page.tsx b/pages/button/performance-marks.page.tsx index 89ff38b71a..95ab690ef5 100644 --- a/pages/button/performance-marks.page.tsx +++ b/pages/button/performance-marks.page.tsx @@ -36,8 +36,9 @@ export default function ButtonsPerformanceMarkPage() { + Submit modal}> diff --git a/src/button/__integ__/performance-marks.test.ts b/src/button/__integ__/performance-marks.test.ts index 0c9d979411..d531ce5346 100644 --- a/src/button/__integ__/performance-marks.test.ts +++ b/src/button/__integ__/performance-marks.test.ts @@ -26,7 +26,7 @@ function setupTest( describe('Button', () => { test( - 'Emits a mark only for primary visible buttons', + 'Emits a mark only for primary visible buttons which are loaded', setupTest('performance-marks', async ({ getMarks, getElementPerformanceMarkText }) => { const marks = await getMarks(); @@ -35,7 +35,7 @@ describe('Button', () => { expect(marks[0].detail).toMatchObject({ source: 'awsui', instanceIdentifier: expect.any(String), - loading: true, + loading: false, disabled: false, text: 'Primary button', }); @@ -47,32 +47,33 @@ describe('Button', () => { test( 'Emits a mark when properties change', setupTest('performance-marks', async ({ page, getMarks, getElementPerformanceMarkText }) => { - await page.click('#disabled'); await page.click('#loading'); const marks = await getMarks(); - expect(marks).toHaveLength(3); - expect(marks[1].name).toBe('primaryButtonUpdated'); - expect(marks[1].detail).toMatchObject({ + expect(marks).toHaveLength(2); + expect(marks[0].name).toBe('primaryButtonRendered'); + expect(marks[0].detail).toMatchObject({ source: 'awsui', instanceIdentifier: marks[0].detail.instanceIdentifier, - loading: true, - disabled: true, + loading: false, + disabled: false, text: 'Primary button', }); - await expect(getElementPerformanceMarkText(marks[1].detail.instanceIdentifier)).resolves.toBe('Primary button'); + await expect(getElementPerformanceMarkText(marks[0].detail.instanceIdentifier)).resolves.toBe('Primary button'); - expect(marks[2].name).toBe('primaryButtonUpdated'); - expect(marks[2].detail).toMatchObject({ + expect(marks[1].name).toBe('primaryButtonUpdated'); + expect(marks[1].detail).toMatchObject({ source: 'awsui', instanceIdentifier: marks[1].detail.instanceIdentifier, loading: false, - disabled: true, - text: 'Primary button', + disabled: false, + text: 'Primary button with loading and disabled props', }); - await expect(getElementPerformanceMarkText(marks[2].detail.instanceIdentifier)).resolves.toBe('Primary button'); + await expect(getElementPerformanceMarkText(marks[1].detail.instanceIdentifier)).resolves.toBe( + 'Primary button with loading and disabled props' + ); }) ); @@ -92,7 +93,7 @@ describe('Button', () => { ); test( - 'Emits a mark when evaluateComponentVisibility event for button components', + 'Emits a mark when evaluateComponentVisibility event for loaded button components', setupTest('performance-marks', async ({ page, getMarks, getElementPerformanceMarkText }) => { let marks = await getMarks(); expect(marks).toHaveLength(1); @@ -106,7 +107,7 @@ describe('Button', () => { expect(marks[0].detail).toMatchObject({ source: 'awsui', instanceIdentifier: marks[0].detail.instanceIdentifier, - loading: true, + loading: false, disabled: false, text: 'Primary button', }); @@ -117,7 +118,7 @@ describe('Button', () => { expect(marks[1].detail).toMatchObject({ source: 'awsui', instanceIdentifier: marks[1].detail.instanceIdentifier, - loading: true, + loading: false, disabled: false, text: 'Primary button', }); diff --git a/src/button/internal.tsx b/src/button/internal.tsx index c07351db16..adf6b82d22 100644 --- a/src/button/internal.tsx +++ b/src/button/internal.tsx @@ -124,7 +124,7 @@ export const InternalButton = React.forwardRef( const performanceMarkAttributes = usePerformanceMarks( 'primaryButton', - variant === 'primary' && __emitPerformanceMarks, + () => variant === 'primary' && __emitPerformanceMarks && !loading && !disabled, buttonRef, () => ({ loading, diff --git a/src/internal/hooks/use-performance-marks/__tests__/use-performance-marks.test.tsx b/src/internal/hooks/use-performance-marks/__tests__/use-performance-marks.test.tsx index 0ae5738662..cb0d4a2efd 100644 --- a/src/internal/hooks/use-performance-marks/__tests__/use-performance-marks.test.tsx +++ b/src/internal/hooks/use-performance-marks/__tests__/use-performance-marks.test.tsx @@ -8,7 +8,13 @@ import { usePerformanceMarks } from '../index'; function Demo() { const ref = useRef(null); - const attributes = usePerformanceMarks('test-component', true, ref, () => ({}), []); + const attributes = usePerformanceMarks( + 'test-component', + () => true, + ref, + () => ({}), + [] + ); return
; } diff --git a/src/internal/hooks/use-performance-marks/index.ts b/src/internal/hooks/use-performance-marks/index.ts index d1c7dfb2e1..4fbfce61d3 100644 --- a/src/internal/hooks/use-performance-marks/index.ts +++ b/src/internal/hooks/use-performance-marks/index.ts @@ -41,7 +41,7 @@ const useEvaluateComponentVisibility = () => { */ export function usePerformanceMarks( name: string, - enabled: boolean, + enabled: () => boolean, elementRef: React.RefObject, getDetails: () => Record, dependencies: React.DependencyList @@ -51,7 +51,7 @@ export function usePerformanceMarks( const attributes = useDOMAttribute(elementRef, 'data-analytics-performance-mark', id); const evaluateComponentVisibility = useEvaluateComponentVisibility(); useEffect(() => { - if (!enabled || !elementRef.current || isInModal) { + if (!enabled() || !elementRef.current || isInModal) { return; } const elementVisible = @@ -75,7 +75,7 @@ export function usePerformanceMarks( }, []); useEffectOnUpdate(() => { - if (!enabled || !elementRef.current || isInModal) { + if (!enabled() || !elementRef.current || isInModal) { return; } const elementVisible = diff --git a/src/table/__integ__/performance-marks.test.ts b/src/table/__integ__/performance-marks.test.ts index 2c05230375..37737035e3 100644 --- a/src/table/__integ__/performance-marks.test.ts +++ b/src/table/__integ__/performance-marks.test.ts @@ -27,29 +27,18 @@ function setupTest( describe('Table', () => { test( - 'Emits a mark only for visible tables', + 'Emits a mark only for visible tables which are loaded completely', setupTest(async ({ getMarks, isElementPerformanceMarkExisting }) => { const marks = await getMarks(); - expect(marks).toHaveLength(2); + expect(marks).toHaveLength(1); expect(marks[0].name).toBe('tableRendered'); expect(marks[0].detail).toEqual({ - source: 'awsui', - instanceIdentifier: expect.any(String), - loading: true, - header: 'This is my table', - }); - - expect(marks[1].name).toBe('tableRendered'); - expect(marks[1].detail).toEqual({ source: 'awsui', instanceIdentifier: expect.any(String), loading: false, header: 'A table without the Header component', }); - - expect(marks[0].detail.instanceIdentifier).not.toEqual(marks[1].detail.instanceIdentifier); - await expect(isElementPerformanceMarkExisting(marks[0].detail.instanceIdentifier)).resolves.toBeTruthy(); }) ); @@ -58,47 +47,27 @@ describe('Table', () => { 'Emits a mark when properties change', setupTest(async ({ page, getMarks, isElementPerformanceMarkExisting }) => { await page.click('#loading'); - let marks = await getMarks(); + const marks = await getMarks(); - expect(marks).toHaveLength(3); - expect(marks[2].name).toBe('tableUpdated'); - expect(marks[2].detail).toMatchObject({ + expect(marks).toHaveLength(2); + expect(marks[1].name).toBe('tableUpdated'); + expect(marks[1].detail).toMatchObject({ source: 'awsui', - instanceIdentifier: marks[0].detail.instanceIdentifier, + instanceIdentifier: expect.any(String), loading: false, header: 'This is my table', }); - await expect(isElementPerformanceMarkExisting(marks[2].detail.instanceIdentifier)).resolves.toBeTruthy(); - - await page.click('#loading'); - - marks = await getMarks(); - - expect(marks[3].name).toBe('tableUpdated'); - expect(marks[3].detail).toMatchObject({ - source: 'awsui', - instanceIdentifier: marks[2].detail.instanceIdentifier, - loading: true, - header: 'This is my table', - }); - await expect(isElementPerformanceMarkExisting(marks[2].detail.instanceIdentifier)).resolves.toBeTruthy(); + await expect(isElementPerformanceMarkExisting(marks[1].detail.instanceIdentifier)).resolves.toBeTruthy(); }) ); test( - 'Emits a mark when evaluateComponentVisibility event is emitted', + 'Emits a mark for loaded table components when evaluateComponentVisibility event is emitted', setupTest(async ({ page, getMarks, isElementPerformanceMarkExisting }) => { let marks = await getMarks(); - expect(marks).toHaveLength(2); + expect(marks).toHaveLength(1); expect(marks[0].name).toBe('tableRendered'); expect(marks[0].detail).toEqual({ - source: 'awsui', - instanceIdentifier: expect.any(String), - loading: true, - header: 'This is my table', - }); - expect(marks[1].name).toBe('tableRendered'); - expect(marks[1].detail).toEqual({ source: 'awsui', instanceIdentifier: expect.any(String), loading: false, @@ -108,17 +77,10 @@ describe('Table', () => { await expect(isElementPerformanceMarkExisting(marks[0].detail.instanceIdentifier)).resolves.toBeTruthy(); await page.click('#evaluateComponentVisibility'); marks = await getMarks(); - expect(marks).toHaveLength(4); + expect(marks).toHaveLength(2); - expect(marks[2].name).toBe('tableUpdated'); - expect(marks[2].detail).toEqual({ - source: 'awsui', - instanceIdentifier: expect.any(String), - loading: true, - header: 'This is my table', - }); - expect(marks[3].name).toBe('tableUpdated'); - expect(marks[3].detail).toEqual({ + expect(marks[1].name).toBe('tableUpdated'); + expect(marks[1].detail).toEqual({ source: 'awsui', instanceIdentifier: expect.any(String), loading: false, diff --git a/src/table/internal.tsx b/src/table/internal.tsx index 207c8ec616..fec6c238b6 100644 --- a/src/table/internal.tsx +++ b/src/table/internal.tsx @@ -201,7 +201,7 @@ const InternalTable = React.forwardRef( const performanceMarkAttributes = usePerformanceMarks( 'table', - true, + () => !loading, tableRefObject, () => ({ loading: loading ?? false,