Skip to content

Commit

Permalink
fix: Emit performance marks only for loaded components (#3175)
Browse files Browse the repository at this point in the history 
Co-authored-by: Abdallah AlHalees <[email protected]>
  • Loading branch information
@guptbhum @abdhalees
guptbhum and abdhalees authored Jan 10, 2025
1 parent 1f1d269 commit 67c06fa
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 75 deletions.
3 changes: 2 additions & 1 deletion pages/button/performance-marks.page.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ export default function ButtonsPerformanceMarkPage() {
</Button>
</label>
<Button variant="primary" loading={loading} disabled={disabled}>
Primary button
Primary button with loading and disabled props
</Button>
<Button variant="primary">Primary button</Button>
<Button>Non-primary button</Button>

<Modal visible={false} footer={<Button variant="primary">Submit modal</Button>}></Modal>
Expand Down
35 changes: 18 additions & 17 deletions src/button/__integ__/performance-marks.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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',
});
Expand All @@ -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'
);
})
);

Expand All @@ -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);
Expand All @@ -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',
});
Expand All @@ -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',
});
Expand Down
2 changes: 1 addition & 1 deletion src/button/internal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export const InternalButton = React.forwardRef(

const performanceMarkAttributes = usePerformanceMarks(
'primaryButton',
variant === 'primary' && __emitPerformanceMarks,
() => variant === 'primary' && __emitPerformanceMarks && !loading && !disabled,
buttonRef,
() => ({
loading,
Expand Down
8 changes: 7 additions & 1 deletion src/internal/hooks/use-performance-marks/__tests__/use-performance-marks.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@ import { usePerformanceMarks } from '../index';

function Demo() {
const ref = useRef<HTMLDivElement>(null);
const attributes = usePerformanceMarks('test-component', true, ref, () => ({}), []);
const attributes = usePerformanceMarks(
'test-component',
() => true,
ref,
() => ({}),
[]
);
return <div {...attributes} ref={ref} data-testid="element" />;
}

Expand Down
6 changes: 3 additions & 3 deletions src/internal/hooks/use-performance-marks/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const useEvaluateComponentVisibility = () => {
*/
export function usePerformanceMarks(
name: string,
enabled: boolean,
enabled: () => boolean,
elementRef: React.RefObject<HTMLElement>,
getDetails: () => Record<string, string | boolean | number | undefined>,
dependencies: React.DependencyList
Expand All @@ -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 =
Expand All @@ -75,7 +75,7 @@ export function usePerformanceMarks(
}, []);

useEffectOnUpdate(() => {
if (!enabled || !elementRef.current || isInModal) {
if (!enabled() || !elementRef.current || isInModal) {
return;
}
const elementVisible =
Expand Down
64 changes: 13 additions & 51 deletions src/table/__integ__/performance-marks.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
})
);
Expand All @@ -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,
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/table/internal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ const InternalTable = React.forwardRef(

const performanceMarkAttributes = usePerformanceMarks(
'table',
true,
() => !loading,
tableRefObject,
() => ({
loading: loading ?? false,
Expand Down

0 comments on commit 67c06fa

Please sign in to comment.