diff --git a/docs/troubleshooting_no_data.md b/docs/troubleshooting_no_data.md index dca2f8e11..06c83ea70 100644 --- a/docs/troubleshooting_no_data.md +++ b/docs/troubleshooting_no_data.md @@ -71,17 +71,29 @@ import * as NativeWindJsxRuntime from 'nativewind/jsx-runtime'; config.jsxRuntimes = [NativeWindJsxRuntime]; ``` -A runtime's factories may be exposed through accessors rather than plain properties, depending -on how the module was built and how your bundler models the import. The SDK replaces them -either way, and only gives up when the property is also non-configurable — a frozen ES module -namespace, for instance. That case is logged as: +A runtime may expose its factories through accessors rather than plain properties. The SDK +leaves those alone, on purpose: a host that puts a getter there is managing the slot, not just +storing a function in it, and taking it over breaks bookkeeping the host still believes it +controls. It is logged as: + +``` +Datadog SDK won't replace "createElement": it is an accessor, so the host framework owns that slot +``` + +Seeing this for `createElement` or `memo` on a nativewind app is expected and harmless — under +the automatic JSX transform your components never call `React.createElement`, so instrumenting +the runtime you declared in `jsxRuntimes` is what actually matters. One side effect worth +knowing: with `memo` left alone, a memoized component whose `onPress` the SDK wraps will +re-render on every parent render, because the wrapper is a new function each time. + +When no factory on a runtime could be replaced at all, the SDK says what it costs: ``` Datadog SDK could not instrument a JSX runtime: its element factories are read-only. No RUM action will be recorded for elements it renders. ``` -There is no configuration that recovers from it: the factories have to be instrumented while +There is no configuration that recovers from that: the factories have to be instrumented while the app is built instead. Two things to try, in order — import the runtime with `require()` rather than `import * as`, which skips the interop layer that may have frozen it, and if that still fails, open an issue. Views, resources and errors are unaffected either way. diff --git a/packages/core/src/__tests__/rum/instrumentation/DdRumUserInteractionTracking.test.tsx b/packages/core/src/__tests__/rum/instrumentation/DdRumUserInteractionTracking.test.tsx index 4540c1a5d..12472ff00 100644 --- a/packages/core/src/__tests__/rum/instrumentation/DdRumUserInteractionTracking.test.tsx +++ b/packages/core/src/__tests__/rum/instrumentation/DdRumUserInteractionTracking.test.tsx @@ -484,9 +484,11 @@ describe('startTracking with injected jsx runtimes', () => { expect(DdRumUserInteractionTracking['isTracking']).toBe(true); }); - it('M patch through an accessor W the property is configurable', async () => { - // a namespace object built by an interop helper can expose accessors rather than - // plain properties; assignment does nothing there, redefining still works + it('M leave an accessor alone W the host owns that slot', async () => { + // A host that exposes a factory through a getter is managing that slot, not just + // storing a function in it. Redefining it does succeed and then the host's own + // bookkeeping operates on something it no longer controls - on a nativewind app that + // showed up as the heap growing until Hermes aborted at startup. const runtime = {}; const original = jest.fn(); Object.defineProperty(runtime, 'jsx', { @@ -497,6 +499,22 @@ describe('startTracking with injected jsx runtimes', () => { DdRumUserInteractionTracking.startTracking({}, [runtime]); + expect((runtime as any).jsx).toBe(original); + }); + + it('M redefine a plain read-only property W it is configurable', async () => { + // no getter means no host logic behind the slot, so taking it over is safe + const runtime = {}; + const original = jest.fn(); + Object.defineProperty(runtime, 'jsx', { + value: original, + writable: false, + enumerable: true, + configurable: true + }); + + DdRumUserInteractionTracking.startTracking({}, [runtime]); + expect((runtime as any).jsx).not.toBe(original); const onPress = jest.fn(); diff --git a/packages/core/src/rum/instrumentation/interactionTracking/DdRumUserInteractionTracking.tsx b/packages/core/src/rum/instrumentation/interactionTracking/DdRumUserInteractionTracking.tsx index f78e7fd8f..8d3c4226f 100644 --- a/packages/core/src/rum/instrumentation/interactionTracking/DdRumUserInteractionTracking.tsx +++ b/packages/core/src/rum/instrumentation/interactionTracking/DdRumUserInteractionTracking.tsx @@ -38,28 +38,41 @@ type PatchedRuntime = { /** * Replaces a property, and reports whether it actually took. * - * A host framework can expose its element factories through accessors rather than plain - * properties. Plain assignment then silently does nothing in sloppy mode and throws in strict - * mode, so this checks the result instead of trusting either, and falls back to redefining the - * property - which still works as long as it is configurable. + * An accessor is left alone on purpose. A host framework that exposes an element factory + * through a getter is not merely storing a function there - it is managing that slot, and the + * value it returns participates in bookkeeping of its own. Redefining the slot as a plain + * property does succeed, and then that bookkeeping silently operates on something the host no + * longer controls; the observed result on a nativewind app was the heap growing without bound + * until Hermes aborted. Not writable is not an obstacle to push harder against, it is the host + * saying this is mine. * - * This runs inside `enableFeatures`, where a throw used to take resource and error tracking - * down with it and, through `DatadogProvider`, abort the native initialization that follows. - * Auto-instrumentation is best-effort: failing to patch one factory must never be the reason - * the SDK does not start. + * A plain property that merely refuses writes carries no such logic, so redefining that one is + * safe and still worth doing. + * + * Either way this must not throw: it runs inside `enableFeatures`, where an exception used to + * take the other instrumentations - and the native initialization after them - down with it. */ const replaceProperty = ( target: Record, key: string, value: unknown ): boolean => { + const descriptor = Object.getOwnPropertyDescriptor(target, key); + if (descriptor && (descriptor.get || descriptor.set)) { + InternalLog.log( + `Datadog SDK won't replace "${key}": it is an accessor, so the host framework owns that slot`, + SdkVerbosity.WARN + ); + return false; + } + try { target[key] = value; if (target[key] === value) { return true; } } catch (error) { - // accessor without a setter in strict mode - fall through to defineProperty + // read-only in strict mode - redefining is still allowed if it is configurable } try { @@ -92,8 +105,14 @@ const reactModule = (React as unknown) as Record; export class DdRumUserInteractionTracking { private static isTracking = false; private static eventsInterceptor: EventsInterceptor = new NoOpEventsInterceptor(); - private static originalCreateElement = React.createElement; - private static originalMemo = React.memo; + // Read through the module object, never as `React.createElement`. A host's Babel plugin + // can rewrite that member expression to its own factory - react-native-css-interop does - + // and then what gets saved here is the host's wrapper while what gets replaced below is + // React's own property. Every createElement call in the app, React's internals included, + // would be routed through the host's wrapper: input it never expects. Bracket access is + // invisible to such a plugin, so both sides stay on the same object. + private static originalCreateElement = reactModule['createElement']; + private static originalMemo = reactModule['memo']; private static patchedRuntimes: PatchedRuntime[] = []; private static patchCreateElementFunction = ( @@ -224,7 +243,7 @@ export class DdRumUserInteractionTracking { options ); - const originalCreateElement = React.createElement; + const originalCreateElement = reactModule['createElement']; replaceProperty( reactModule, 'createElement', @@ -251,7 +270,7 @@ export class DdRumUserInteractionTracking { runtimes.push(...jsxRuntimes); runtimes.forEach(DdRumUserInteractionTracking.patchJsxRuntime); - const originalMemo = React.memo; + const originalMemo = reactModule['memo']; replaceProperty( reactModule, 'memo', @@ -259,7 +278,7 @@ export class DdRumUserInteractionTracking { component: any, propsAreEqual?: (prevProps: any, newProps: any) => boolean ) => { - return originalMemo(component, (prev, next) => { + return originalMemo(component, (prev: any, next: any) => { if (!next.onPress || !prev.onPress) { return propsAreEqual ? propsAreEqual(prev, next)