Skip to content

Commit

Permalink
feat: avoid memory leak when caching LCP candidates
Browse files Browse the repository at this point in the history
  • Loading branch information
williazz committed Sep 27, 2023
1 parent 42417b0 commit 2f70841
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 2 deletions.
9 changes: 9 additions & 0 deletions src/orchestration/__tests__/Orchestration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ jest.mock('../../dispatch/Dispatch', () => ({
}))
}));

jest.mock('../../utils/common-utils', () => {
const originalModule = jest.requireActual('../../utils/common-utils');
return {
__esModule: true,
...originalModule,
isLCPSupported: jest.fn().mockReturnValue(true)
};
});

const enableEventCache = jest.fn();
const disableEventCache = jest.fn();
const recordPageView = jest.fn();
Expand Down
9 changes: 7 additions & 2 deletions src/plugins/event-plugins/WebVitalsPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import {
HasLatency,
ResourceType,
performanceKey,
RumLCPAttribution
RumLCPAttribution,
isLCPSupported
} from '../../utils/common-utils';

export const WEB_VITAL_EVENT_PLUGIN_ID = 'web-vitals';
Expand All @@ -36,6 +37,7 @@ export class WebVitalsPlugin extends InternalPlugin {
}
private resourceEventIds = new Map<string, string>();
private navigationEventId?: string;
private cacheLCPCandidates = isLCPSupported();

// eslint-disable-next-line @typescript-eslint/no-empty-function
enable(): void {}
Expand All @@ -58,7 +60,10 @@ export class WebVitalsPlugin extends InternalPlugin {
// lcp resource is either image or text
case PERFORMANCE_RESOURCE_EVENT_TYPE:
const details = event.details as ResourceEvent;
if (details.fileType === ResourceType.IMAGE) {
if (
this.cacheLCPCandidates &&
details.fileType === ResourceType.IMAGE
) {
this.resourceEventIds.set(
performanceKey(event.details as HasLatency),
event.id
Expand Down
35 changes: 35 additions & 0 deletions src/plugins/event-plugins/__tests__/WebVitalsPlugin.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
jest.mock('../../../utils/common-utils', () => {
const originalModule = jest.requireActual('../../../utils/common-utils');
return {
__esModule: true,
...originalModule,
isLCPSupported: jest.fn().mockReturnValue(true)
};
});
import { ResourceType } from '../../../utils/common-utils';
import {
CLS_EVENT_TYPE,
Expand Down Expand Up @@ -237,6 +245,33 @@ describe('WebVitalsPlugin tests', () => {
// restore
imageResourceRumEvent.details.fileType = fileType;
});

test('when lcp is recorded then cache is empty', async () => {
const plugin = new WebVitalsPlugin();

plugin.load(context);
expect(record).toHaveBeenCalledWith(LCP_EVENT_TYPE, expect.anything());
expect((plugin as any).resourceEventIds.size).toEqual(0);
expect((plugin as any).navigationEventId).toBeUndefined();
});

test('when lcp is not supported then cache is empty', async () => {
const plugin = new WebVitalsPlugin();
(plugin as any).cacheLCPCandidates = false;

plugin.load(context);
expect(record).toHaveBeenCalledWith(
LCP_EVENT_TYPE,
expect.objectContaining({
attribution: expect.not.objectContaining({
lcpResourceEntry: expect.anything()
})
})
);
expect((plugin as any).resourceEventIds.size).toEqual(0);
expect((plugin as any).navigationEventId).toBeUndefined();
});

test('when lcp is recorded then unsubscribe is called', async () => {
// init
const unsubscribeSpy = jest.spyOn(context.eventBus, 'unsubscribe');
Expand Down
15 changes: 15 additions & 0 deletions src/utils/common-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,18 @@ export interface RumLCPAttribution {
lcpResourceEntry?: string;
navigationEntry?: string;
}

/** Checks at runtime if the web vitals package will record LCP
* If PerformanceAPI ever changes this API, or if WebVitals package implements a polyfill,
* then this needs to be updated
*
* Reference code from web vitals package:
* https://github.com/GoogleChrome/web-vitals/blob/main/src/lib/observe.ts#L46
* Discussion for context:
* https://github.com/aws-observability/aws-rum-web/pull/448#issuecomment-1734314463
*/
export const isLCPSupported = () => {
return PerformanceObserver.supportedEntryTypes.includes(
'largest-contentful-paint'
);
};

0 comments on commit 2f70841

Please sign in to comment.