Repository navigation
fix: do not break app startup on an Electron without registerPreloadScript - #27
Merged
Merged
Conversation
…cript `session.registerPreloadScript` was added in Electron 35. Both this SDK and dd-trace's `BrowserWindow` subclass call it unconditionally -- the SDK from an `app` 'ready' listener, dd-trace from every `new BrowserWindow()` -- so on an older runtime the missing method threw where the host application had no way to catch it. The instrumentation entry point runs before the application's own 'ready' handler, so the failure preempted window creation and the application died at startup with `Cannot read properties of undefined (reading 'bind')`. `peerDependencies: electron >= 39` does not prevent this: it is an installation error only under npm, and a warning under Yarn and pnpm. Install a stand-in for the missing method at the session boundary, before anything reads it, so dd-trace's call is harmless too, and warn once with the cause. Losing the renderer bridge is what an unsupported Electron costs: main process monitoring is unaffected and the Browser SDK keeps collecting in renderers, though renderer events do not share the main process session. Also wrap the listeners the SDK puts on the host application in `monitor()`, so no failure inside them can propagate into the application again.
…ion site Both listeners the SDK registers on the host application end up in `setUp`, so wrapping it once covers the 'session-created' event, the deferred 'ready' listener and the synchronous `isReady()` branch alike. This is also how the rest of the SDK guards its listeners -- at the definition, not per registration.
`datadog-instrumentations` gates its whole Electron hook on `electron >= 37`, so dd-trace never wraps `BrowserWindow` — and never registers a preload — on the versions this failure is about. The startup failure came from the SDK's own listeners alone; the stand-in and the `monitor()` wrappers are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
session.registerPreloadScriptwas added in Electron 35. The SDK registers the bridge preload through it from anapp'ready'listener and from'session-created'— neither of which the host application can guard — so on an older runtime the missing method threw where nothing could catch it.The instrumentation entry point runs before the application's own
'ready'handler, so the failure preempted window creation: the application died at startup withwhich says nothing about the Electron version behind it.
peerDependencies: electron >= 39does not prevent this. It is an installation error only under npm; Yarn and pnpm merely warn and install.(dd-trace registers a preload of its own the same way, but
datadog-instrumentationsgates its whole Electron hook onelectron >= 37, so on the versions this is about it installs nothing.)Change
monitor(), atsetUp's definition, so no failure inside them can propagate into the application again.An unsupported Electron now loses the renderer bridge instead of the application: main process monitoring is unaffected, and the Browser SDK keeps collecting in renderers, though renderer events do not share the main process session.
Verification
installBridgePreloadwith a session shaped like Electron < 35 (noregisterPreloadScript). Reverting the source change makes all five fail with the exactTypeErrorabove.'ready'path, thesession-createdpath, theisReady()-true path, a subsequent registration through the patched method, and warn-once across sessions.tsc --noEmitandeslintclean; CI green.