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

feat(core): Deprecate API class #4281

Merged
merged 8 commits into from
Dec 14, 2021
Merged

feat(core): Deprecate API class #4281

merged 8 commits into from
Dec 14, 2021

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Dec 14, 2021

Trying to minimize behaviour differences until we properly refactor the transports as per Extract out transports into their own modules in #4240

@AbhiPrasad AbhiPrasad added this to the Treeshaking / Bundle Size milestone Dec 14, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2021

size-limit report

Path Base Size (f042723) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 21.67 KB 21.44 KB -1.09% 🔽
@sentry/browser - Webpack 23.35 KB 23.25 KB -0.42% 🔽
@sentry/browser - Webpack - gzip = false 82.8 KB 82.15 KB -0.8% 🔽
@sentry/react - Webpack 23.38 KB 23.29 KB -0.41% 🔽
@sentry/nextjs Client - Webpack 48.01 KB 47.92 KB -0.21% 🔽
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.99 KB 29.77 KB -0.73% 🔽

@AbhiPrasad AbhiPrasad marked this pull request as ready for review December 14, 2021 16:21
@@ -38,7 +43,7 @@ export abstract class BaseTransport implements Transport {
public url: string;

/** Helper to get Sentry API endpoints. */
protected readonly _api: API;
protected readonly _api: APIDetails;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay but generally I think this design still makes it hard for us to make improvements later. Unfortunately the subclassing situation will always be bad for minification but quite user friendly for custom transports, so it might be acceptable to do it this way.

In Python as an example we push the original options in (there are no transport options) and the transports pull from there what they need. We should probably not change this right now, but longer term I think that might be preferable here as well.

Copy link
Member Author

@AbhiPrasad AbhiPrasad Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree - but we can revisit this when we examine transports more holistically later on. I kept this pattern primarily to minimize the diff and possibilities for breakage.

original options in (there are no transport options) and the transports pull from there what they need

This is the direction we should head in, actually we might be able to pull it off before the major

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants