From 618727482abe3173559fdf571067512e716b728f Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 14 Mar 2023 20:16:19 +0100 Subject: [PATCH 1/4] chore(sdk): Better DEX when native disabled --- src/js/sdk.tsx | 8 +++++++- src/js/tracing/nativeframes.ts | 4 ++++ src/js/transports/native.ts | 6 ++++-- src/js/wrapper.ts | 16 ++++++++++------ test/sdk.test.ts | 18 ++++++++++++++++-- 5 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/js/sdk.tsx b/src/js/sdk.tsx index 231073e2a..3de55788c 100644 --- a/src/js/sdk.tsx +++ b/src/js/sdk.tsx @@ -67,7 +67,13 @@ export function init(passedOptions: ReactNativeOptions): void { ...DEFAULT_OPTIONS, ...passedOptions, // If custom transport factory fails the SDK won't initialize - transport: passedOptions.transport || makeNativeTransportFactory() || makeFetchTransport, + transport: passedOptions.transport + || makeNativeTransportFactory({ + enableNative: passedOptions.enableNative !== undefined + ? passedOptions.enableNative + : DEFAULT_OPTIONS.enableNative + }) + || makeFetchTransport, transportOptions: { ...DEFAULT_OPTIONS.transportOptions, ...(passedOptions.transportOptions ?? {}), diff --git a/src/js/tracing/nativeframes.ts b/src/js/tracing/nativeframes.ts index c03de6b02..e2e11a158 100644 --- a/src/js/tracing/nativeframes.ts +++ b/src/js/tracing/nativeframes.ts @@ -55,6 +55,8 @@ export class NativeFramesInstrumentation { if (framesMetrics) { transaction.setData('__startFrames', framesMetrics); } + }).then(undefined, (error) => { + logger.error(`[ReactNativeTracing] Error while fetching native frames: ${error}`); }); instrumentChildSpanFinish(transaction, (_: Span, endTimestamp?: number) => { @@ -85,6 +87,8 @@ export class NativeFramesInstrumentation { nativeFrames, }; } + }).then(undefined, (error) => { + logger.error(`[ReactNativeTracing] Error while fetching native frames: ${error}`); }); } diff --git a/src/js/transports/native.ts b/src/js/transports/native.ts index dce3dec44..7b5daa316 100644 --- a/src/js/transports/native.ts +++ b/src/js/transports/native.ts @@ -53,8 +53,10 @@ export function makeNativeTransport(options: BaseNativeTransportOptions = {}): N /** * Creates a Native Transport factory if the native transport is available. */ -export function makeNativeTransportFactory(): typeof makeNativeTransport | null { - if (NATIVE.isNativeTransportAvailable()) { +export function makeNativeTransportFactory( + { enableNative }: { enableNative?: boolean }, +): typeof makeNativeTransport | null { + if (enableNative && NATIVE.isNativeAvailable()) { return makeNativeTransport; } return null; diff --git a/src/js/wrapper.ts b/src/js/wrapper.ts index b4c90e823..0f6f39250 100644 --- a/src/js/wrapper.ts +++ b/src/js/wrapper.ts @@ -53,7 +53,7 @@ interface SentryNativeWrapper { ): module is Spec; _getBreadcrumbs(event: Event): Breadcrumb[] | undefined; - isNativeTransportAvailable(): boolean; + isNativeAvailable(): boolean; initNativeSdk(options: ReactNativeOptions): PromiseLike; closeNativeSdk(): PromiseLike; @@ -267,10 +267,12 @@ export const NATIVE: SentryNativeWrapper = { async fetchNativeAppStart(): Promise { if (!this.enableNative) { - throw this._DisabledNativeError; + logger.warn(this._DisabledNativeError); + return null; } if (!this._isModuleLoaded(RNSentry)) { - throw this._NativeClientError; + logger.error(this._NativeClientError); + return null; } return RNSentry.fetchNativeAppStart(); @@ -468,16 +470,18 @@ export const NATIVE: SentryNativeWrapper = { RNSentry.enableNativeFramesTracking(); }, - isNativeTransportAvailable(): boolean { + isNativeAvailable(): boolean { return this.enableNative && this._isModuleLoaded(RNSentry); }, async captureScreenshot(): Promise { if (!this.enableNative) { - throw this._DisabledNativeError; + logger.warn(this._DisabledNativeError); + return null; } if (!this._isModuleLoaded(RNSentry)) { - throw this._NativeClientError; + logger.error(this._NativeClientError); + return null; } try { diff --git a/test/sdk.test.ts b/test/sdk.test.ts index 1072ee150..6fc6eaba3 100644 --- a/test/sdk.test.ts +++ b/test/sdk.test.ts @@ -228,16 +228,30 @@ describe('Tests the SDK functionality', () => { }); it('uses native transport', () => { - (NATIVE.isNativeTransportAvailable as jest.Mock).mockImplementation(() => true); + (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); init({}); expect(usedOptions()?.transport).toEqual(makeNativeTransport); }); it('uses fallback fetch transport', () => { - (NATIVE.isNativeTransportAvailable as jest.Mock).mockImplementation(() => false); + (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => false); init({}); expect(usedOptions()?.transport).toEqual(makeFetchTransport); }); + + it('checks sdk options first', () => { + (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); + init({ enableNative: false }); + expect(usedOptions()?.transport).toEqual(makeFetchTransport); + expect(NATIVE.isNativeAvailable).not.toBeCalled(); + }); + + it('check both options and native availability', () => { + (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); + init({ enableNative: true }); + expect(usedOptions()?.transport).toEqual(makeNativeTransport); + expect(NATIVE.isNativeAvailable).toBeCalled(); + }); }); describe('initIsSafe', () => { From 218ffd4434b439beabc8a097a5e3716d711f0776 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 14 Mar 2023 20:20:39 +0100 Subject: [PATCH 2/4] Add changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68cd6d93c..a687afe75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixes + +- Fix use Fetch transport when option `enableNative` is `false` +- Improve logs when `enableNative` is `false` + ### Dependencies - Bump JavaScript SDK from v7.40.0 to v7.43.0 ([#2874](https://github.com/getsentry/sentry-react-native/pull/2874)) From 16f2bff201bf339d0e9a260d79c185d137ddb59f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kry=C5=A1tof=20Wold=C5=99ich?= <31292499+krystofwoldrich@users.noreply.github.com> Date: Wed, 15 Mar 2023 12:35:16 +0100 Subject: [PATCH 3/4] Update CHANGELOG.md --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a687afe75..6aa40a745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,8 @@ ### Fixes -- Fix use Fetch transport when option `enableNative` is `false` -- Improve logs when `enableNative` is `false` +- Fix use Fetch transport when option `enableNative` is `false` ([#2897](https://github.com/getsentry/sentry-react-native/pull/2897)) +- Improve logs when `enableNative` is `false` ([#2897](https://github.com/getsentry/sentry-react-native/pull/2897)) ### Dependencies From c3f0cd1c9fb70b1196bd4c618fd71c6894de6c51 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Wed, 15 Mar 2023 12:40:26 +0100 Subject: [PATCH 4/4] fix lint --- test/sdk.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/sdk.test.ts b/test/sdk.test.ts index 6fc6eaba3..c38acea9b 100644 --- a/test/sdk.test.ts +++ b/test/sdk.test.ts @@ -243,6 +243,7 @@ describe('Tests the SDK functionality', () => { (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); init({ enableNative: false }); expect(usedOptions()?.transport).toEqual(makeFetchTransport); + // eslint-disable-next-line @typescript-eslint/unbound-method expect(NATIVE.isNativeAvailable).not.toBeCalled(); }); @@ -250,6 +251,7 @@ describe('Tests the SDK functionality', () => { (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); init({ enableNative: true }); expect(usedOptions()?.transport).toEqual(makeNativeTransport); + // eslint-disable-next-line @typescript-eslint/unbound-method expect(NATIVE.isNativeAvailable).toBeCalled(); }); });