Require cycle fetch

[KrisLau]

Environment

How do you use Sentry?
Sentry SaaS (sentry.io)

Which SDK and version?

"@sentry/react-native": "^3.2.13",
"@sentry/node": "^6.17.8",
"@sentry/tracing": "^6.17.8",

Steps to Reproduce

1. Install sentry and setup according to docs
2. Run app

Expected Result

No require cycle upon running the app

Actual Result

Getting a require cycle warning from sentry-react-native:

Require cycle: node_modules\react-native\Libraries\Network\fetch.js -> node_modules\whatwg-fetch\dist\fetch.umd.js -> node_modules\react-native\Libraries\Network\fetch.js

[marandaneto]

@KrisLau which SDK version? please edit your issue with the but template

[KrisLau]

@marandaneto Done sorry!

[marandaneto]

@KrisLau we'd need to know the Sentry React Native version, See https://github.com/getsentry/sentry-react-native/releases

I don't see anything related to Sentry in the Actual result error message.

@AbhiPrasad Are you aware of such cycle warning with node and/or tracing?

[jennmueng]

@marandaneto @AbhiPrasad Looks like this could be caused because the JS sdk uses window.fetch which in React Native would trigger this warning.

[AbhiPrasad]

Fetch instrumentation is part of both Breadcrumbs integration and BrowserTracing integration where we monkeypatch the fetch instrumentation. We also have it as part of the fetch transport that is exported.

As only the Breadcrumbs is being used with react native, do we have to make any changes there?

[marandaneto]

Fetch instrumentation is part of both Breadcrumbs integration and BrowserTracing integration where we monkeypatch the fetch instrumentation. We also have it as part of the fetch transport that is exported.

As only the Breadcrumbs is being used with react native, do we have to make any changes there?

Thanks for the input, I'm not entirely sure, @jennmueng ideas?

[marandaneto]

Related to facebook/react-native#23130

[marandaneto]

Apparently, we have to do something like #1144
Running madge . --extensions ts,tsx -c --warning gives me:

1. dist/js/tracing/reactnativetracing.d.ts > dist/js/tracing/routingInstrumentation.d.ts
2. src/js/tracing/reactnativetracing.ts > src/js/tracing/routingInstrumentation.ts

[jennmueng]

I think the solution is to have a case for React Native here
where we patch the global fetch instead of window.fetch as window isn't really a thing in RN.

[marandaneto]

I think the solution is to have a case for React Native here where we patch the global fetch instead of window.fetch as window isn't really a thing in RN.

That's a good point but if you look at https://github.com/getsentry/sentry-react-native/pull/1144/files
You can see that the refactoring had nothing to do with fetch, and it fixed the very same warning.
Do you say that we have to change on the JS side or should we monkeypatch the instrument class?

[lobsterkatie]

That's a good point but if you look at #1144 (files)
You can see that the refactoring had nothing to do with fetch, and it fixed the very same warning.

In that case it was a straightforward case of wrapper.ts importing from backend.ts and backend.ts importing from wrapper.ts.

I'm struggling a bit to work it all out in my head, but my gut says that there's something here with both us and whatwg-fetch monkeypatching/polyfilling fetch, and one of us making the original fetch reference point to a patch, and then that patch getting wrapped with something which calls not the patch but the original... IDK, it gets a little fuzzy after that, but maybe that will help spark someone's thinking.

[jennmueng]

@marandaneto @lobsterkatie I've been able to pinpoint the source of the warning, because we also initialize the browser backend in the React native backend, the browser backend will initialize the fetch transport. Initializing that browser backend with a noop transport removes the warning. Will look into this deeper and find the best way to fix this as we will still need to use the fetch transport when native is disabled.

[jennmueng]

Ok after further investigation here are the lines that triggers the require cycle. when the browser backend calls supportsFetch().

I assume it's something with how React Native lazy-initializes some functionality and fetch seems to be one of them. The line calling new Headers() triggers the polyfill from whatwg-fetch to be run, which leads to the require cycle. The warning does not show if you trigger the lazy initialized fetch beforehand by adding a fetch call before

fetch("").catch((e) => {});

Also, calling new Headers() or any of the others later in the lifecycle of the app poses no issue. Which means for some reason our SDK is getting initialized very early on before the fetch polyfill I assume is eventually called by default?

[marandaneto]

@lobsterkatie or @AbhiPrasad Since this relies on the browser SDK, do you have any ideas on how to fix this? thanks.

[AbhiPrasad]

We currently don't have the cycles to investigate as we are focusing on the major JS SDK bump.

The major version does change how transports work, so let's re-visit this after that!

[marandaneto]

Thanks @AbhiPrasad

[marandaneto]

That's fixed by the JS SDK bump, https://github.com/getsentry/sentry-react-native/releases/tag/4.1.0