sentry-react-native icon indicating copy to clipboard operation
sentry-react-native copied to clipboard

`Sentry.init()` is disabling require cycle warnings for the entire app it's integrated in

Open liamjones opened this issue 2 years ago • 4 comments

OS:

  • [ ] Windows
  • [x] MacOS
  • [ ] Linux

Platform:

  • [x] iOS
  • [x] Android

SDK:

  • [x] @sentry/react-native (>= 1.0.0)
  • [ ] react-native-sentry (<= 0.43.2)

SDK version: 5.15.1

react-native version: 0.72.8

Are you using Expo?

  • [ ] Yes
  • [x] No

Are you using sentry.io or on-premise?

  • [x] sentry.io (SaaS)
  • [ ] on-premise

If you are using sentry.io, please post a link to your issue so we can take a look:

N/A

Configuration:

(@sentry/react-native)

Sentry.init({
  dsn: 'https://[email protected]/...'
  // other options aren't relevant to the bug report
});

I have the following issue:

When the client is initialised it tells RN's LogBox to ignore all require cycles in the app it's integrated with: https://github.com/getsentry/sentry-react-native/blob/5.15.1/src/js/utils/ignorerequirecyclelogs.ts#L7

This is not something I was expecting as it is not @sentry/react-native's responsibility (hiding warnings for the rest of the app it's integrated with). In our case, this code was hiding require cycles we had in our code which we wanted to be informed about. Either this should be a much more specific ignore (since the code talks about a fetch require cycle specifically), be functionality that can be opted out of when initialising the SDK, or it should be removed entirely.

For what it's worth, my recommendation is to remove it entirely. Metro has a resolver configuration option for ignoring specific require cycles and it defaults to ignoring everything under node_modules: https://metrobundler.dev/docs/configuration/#requirecycleignorepatterns. This has been present in Metro since 0.71.2: https://github.com/facebook/metro/commit/100ca6ffcc3ab99b82fac87b41be708040be2e9e. I believe the default ignore pattern would already be hiding any Sentry<->RN<->fetch require cycle.

Steps to reproduce: Create a require cycle between two files:

// a.ts
import {cycleB} from './b'
export const cycleA = 'abc'
console.log(cycleB)

// b.ts
import {cycleA} from './a'
export const cycleB = 'def'
console.log(cycleA)

Then import after Sentry.init call in your app. Observe metro's console log while loading the app

Actual result:

No output about require cycle

Expected result:

WARN Require cycle: path/to/a.ts -> path/to/b.ts -> path/to/a.ts

liamjones avatar Dec 21 '23 15:12 liamjones

Hey @liamjones - thank you for the detailed explanation and the reasoning, highly appreciated! I agree that this is not ideal and we will follow up here, but please be aware that due to PTOs and the holidays this will take a bit longer than usual.

kahest avatar Dec 21 '23 15:12 kahest

Thanks for the quick reply @kahest. No worries about the delay - I will patch-package the code out for us for now. 👍

liamjones avatar Dec 21 '23 15:12 liamjones

Hi @liamjones, thank you for the info, I checked the metro version shipped with RN, RN 0.70 shipped with metro@72, the version before has metro@70.

I believe the best way forward is to remove https://github.com/getsentry/sentry-react-native/blob/c968ff930497b12ca8549cc000096c3c1395ab22/src/js/utils/ignorerequirecyclelogs.ts#L7 for RN 0.70 and newer. And keep it for older RN version 0.69 and older until the Sentry RN SDK removes support for the older version without the Metro require cycles patch.

krystofwoldrich avatar Jan 04 '24 12:01 krystofwoldrich

@krystofwoldrich sounds sensible, 0.70 is already unsupported as far as RN is concerned: https://github.com/reactwg/react-native-releases#which-versions-are-currently-supported

From what I recall, LogBox.ignoreLogs also supports regexp ignores, so you might be able to tighten the ignore for older versions. Maybe it's not worth it if it only applies to unsupported versions of RN though...

liamjones avatar Jan 04 '24 13:01 liamjones