Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚡️ [RUM-6813] Lazy load session replay #3152

Draft
wants to merge 8 commits into
base: v6
Choose a base branch
from
+350 −149
Draft

⚡️ [RUM-6813] Lazy load session replay #3152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a79ab4a
Configure ts module to ES2020
amortemousque Nov 20, 2024
a2e770a
Lazy load session replay
amortemousque Nov 20, 2024
a2ca8f0
Update test app target to fix ssr
amortemousque Nov 21, 2024
c6c30b2
Update the deploy script to deploy chunks
amortemousque Nov 22, 2024
2205ee9
Fix e2e by expose the recorder chunk in the e2e server app
amortemousque Nov 22, 2024
f101a0f
Simplify asyncRunOnReadyState
amortemousque Nov 26, 2024
3e1bc2a
Update chunks format
amortemousque Nov 28, 2024
04ff36b
Upload chunks source map
amortemousque Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/core/src/browser/runOnReadyState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,12 @@ export function runOnReadyState(
const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED
return addEventListener(configuration, window, eventName, callback, { once: true })
}

export function asyncRunOnReadyState(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I introduced an async version of runOnReadyState but if we agree on this lazy loading approach I'll refactor the original runOnReadyState to use promises.

configuration: Configuration,
expectedReadyState: 'complete' | 'interactive'
): Promise<void> {
return new Promise((resolve) => {
runOnReadyState(configuration, expectedReadyState, resolve)
})
}
4 changes: 2 additions & 2 deletions packages/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export {
addTelemetryUsage,
drainPreStartTelemetry,
} from './domain/telemetry'
export { monitored, monitor, callMonitored, setDebugMode } from './tools/monitor'
export { monitored, monitor, callMonitored, setDebugMode, monitorError } from './tools/monitor'
export { Observable, Subscription } from './tools/observable'
export {
startSessionManager,
Expand Down Expand Up @@ -82,7 +82,7 @@ export { AbstractLifeCycle } from './tools/abstractLifeCycle'
export * from './domain/eventRateLimiter/createEventRateLimiter'
export * from './tools/utils/browserDetection'
export { sendToExtension } from './tools/sendToExtension'
export { runOnReadyState } from './browser/runOnReadyState'
export { runOnReadyState, asyncRunOnReadyState } from './browser/runOnReadyState'
export { getZoneJsOriginalValue } from './tools/getZoneJsOriginalValue'
export { instrumentMethod, instrumentSetter, InstrumentedMethodCall } from './tools/instrumentMethod'
export { computeRawError, getFileFromStackTraceString, NO_ERROR_STACK_PRESENT_MESSAGE } from './domain/error/error'
Expand Down
18 changes: 11 additions & 7 deletions packages/core/src/tools/monitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,17 @@ export function callMonitored<T extends (...args: any[]) => any>(
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return fn.apply(context, args)
} catch (e) {
displayIfDebugEnabled(e)
if (onMonitorErrorCollected) {
try {
onMonitorErrorCollected(e)
} catch (e) {
displayIfDebugEnabled(e)
}
monitorError(e)
}
}

export function monitorError(e: unknown) {
displayIfDebugEnabled(e)
if (onMonitorErrorCollected) {
try {
onMonitorErrorCollected(e)
} catch (e) {
displayIfDebugEnabled(e)
}
}
}
Expand Down
31 changes: 31 additions & 0 deletions packages/core/test/collectAsyncCalls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,34 @@ export function collectAsyncCalls<F extends jasmine.Func>(
fail(`Unexpected extra call for spec '${currentSpec!.fullName}'`)
}
}
export function asyncCollectAsyncCalls<F extends jasmine.Func>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 thought: ‏Not a big fan of that function name. Maybe we could use the same function as above, but also return a Promise

export function collectAsyncCalls<F extends jasmine.Func>(
  spy: jasmine.Spy<F>,
  expectedCallsCount: number,
  callback?: (calls: jasmine.Calls<F>) => void
): Promise<jasmine.Calls<F>> {
  ...
}

and deprecate callback usage so we can remove it one day.

Copy link
Contributor Author

@amortemousque amortemousque Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! Sry my intention was not clear. I introduced asyncCollectAsyncCalls for the early review, to validate the approach. My goal is to turn collectAsyncCalls in promise in a dedicated PR before merging this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice!

spy: jasmine.Spy<F>,
expectedCallsCount: number
): Promise<jasmine.Calls<F>> {
return new Promise((resolve, reject) => {
const currentSpec = getCurrentJasmineSpec()
if (!currentSpec) {
reject(new Error('collectAsyncCalls should be called within jasmine code'))
return
}

const checkCalls = () => {
if (spy.calls.count() === expectedCallsCount) {
spy.and.callFake(extraCallDetected as F)
resolve(spy.calls)
} else if (spy.calls.count() > expectedCallsCount) {
extraCallDetected()
}
}

checkCalls()

spy.and.callFake((() => {
checkCalls()
}) as F)

function extraCallDetected() {
reject(new Error(`Unexpected extra call for spec '${currentSpec!.fullName}'`))
}
})
}
1 change: 0 additions & 1 deletion packages/core/tsconfig.esm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"compilerOptions": {
"baseUrl": ".",
"declaration": true,
"module": "es6",
"rootDir": "./src/",
"outDir": "./esm/"
},
Expand Down
1 change: 0 additions & 1 deletion packages/rum-core/tsconfig.esm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"compilerOptions": {
"baseUrl": ".",
"declaration": true,
"module": "es6",
"rootDir": "./src/",
"outDir": "./esm/"
},
Expand Down
1 change: 0 additions & 1 deletion packages/rum-react/tsconfig.esm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"compilerOptions": {
"baseUrl": ".",
"declaration": true,
"module": "es6",
"rootDir": "./src/",
"outDir": "./esm/"
},
Expand Down
1 change: 0 additions & 1 deletion packages/rum-slim/tsconfig.esm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"compilerOptions": {
"baseUrl": ".",
"declaration": true,
"module": "es6",
"rootDir": "./src/",
"outDir": "./esm/"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/rum/src/boot/lazyLoadRecorder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function lazyLoadRecorder() {
return import(/* webpackChunkName: "recorder" */ './startRecording').then((module) => module.startRecording)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: ‏ I have a doubt, does this means that customer need to use webpack as well? or is this a stupid question?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, I used the webpackChunkName special comment to have a fixed chunk name to deploy our CDN files. For customers using NPM:

  • if they use webpack: the chunk will have a fix name
  • If they use another bundler this comment is ignored

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use a fixed name for chunks though. It should include a hash or something to avoid cache issues, because I don't trust cloudfront/the browser to invalidate both files exactly at the same time.

We can talk about it.

}
Loading