getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.86k stars 1.55k forks source link

Circular dependency #4742

Closed marandaneto closed 2 years ago

marandaneto commented 2 years ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/browser

SDK Version

6.17.9

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

Run: madge . --extensions ts,tsx,js -c

More context https://github.com/getsentry/sentry-react-native/issues/2080

Expected Result

No circular dependency found!

Actual Result

✖ Found 6 circular dependencies!

1) packages/ember/addon/ember.d.ts 2) packages/nextjs/src/config/types.ts > packages/nextjs/src/config/webpack.ts 3) packages/types/src/event.ts > packages/types/src/scope.ts > packages/types/src/eventprocessor.ts 4) packages/types/src/span.ts > packages/types/src/transaction.ts 5) packages/types/src/client.ts > packages/types/src/integration.ts > packages/types/src/hub.ts 6) packages/types/src/integration.ts > packages/types/src/hub.ts

AbhiPrasad commented 2 years ago

Circular dependencies in types are not a problem as they still get compiled properly by the typescript compiler. This is why we don't run our circular dependency check on @sentry/types. Why did we raise this?

marandaneto commented 2 years ago

Because Metro warns for the React Native SDK and it's not possible to ignore it, yet.

AbhiPrasad commented 2 years ago

Even for types? The generated d.ts files should have no issues. There are no circular dependencies in the compiled dist assets (plain JS) that the SDK ships with. Is this metro warning for SDK development, or users asking about this?

marandaneto commented 2 years ago

@AbhiPrasad the linked issue in the issue description is opened by a user, that issue is linked to previous issues (even Sentry), and most of the time the circular dependencies are resolved by applying such fixes, if that solves the issue for good? no idea. Today, by installing Sentry in your RN App, you get a circular dependency warn.

github-actions[bot] commented 2 years ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

marandaneto commented 2 years ago

If the Circular dependency issues found by the madge tooling aren't a problem, feel free to close this issue.

AbhiPrasad commented 2 years ago

Yes, all of these are for types, which shouldn't affect the final emitted javascript assets, so we should be fine to close.