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.

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

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

Closed
6 of 11 tasks
liamjones opened this issue Dec 21, 2023 · 4 comments · Fixed by #4214
Closed
6 of 11 tasks

Comments

@liamjones
Copy link
Contributor

OS:

  • Windows
  • MacOS
  • Linux

Platform:

  • iOS
  • Android

SDK:

  • @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
  • No

Are you using sentry.io or on-premise?

  • 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://...@sentry.io/...'
  // 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: facebook/metro@100ca6f. 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

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Dec 21, 2023
@liamjones liamjones changed the title @sentry/react-native should not be disabling require cycle warnings for the entire app it's integrated in Sentry.init() is disabling require cycle warnings for the entire app it's integrated in Dec 21, 2023
@liamjones liamjones changed the title Sentry.init() is disabling require cycle warnings for the entire app it's integrated in Sentry.init() is disabling require cycle warnings for the entire app it's integrated in Dec 21, 2023
@kahest
Copy link
Member

kahest commented Dec 21, 2023

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.

@liamjones
Copy link
Contributor Author

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

@krystofwoldrich
Copy link
Member

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

LogBox.ignoreLogs(['Require cycle:']);
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.

@liamjones
Copy link
Contributor Author

@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...

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 4, 2024
@getsantry getsantry bot removed the status in GitHub Issues with 👀 2 Jan 4, 2024
@krystofwoldrich krystofwoldrich moved this from Needs Discussion to Todo in Mobile & Cross Platform SDK Jan 12, 2024
@antonis antonis self-assigned this Oct 29, 2024
@antonis antonis moved this from Todo to In Progress in Mobile & Cross Platform SDK Oct 29, 2024
@antonis antonis moved this from In Progress to Needs Review in Mobile & Cross Platform SDK Oct 30, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in Mobile & Cross Platform SDK Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants