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

ref(dsn): trim dsn size #4325

Merged
merged 15 commits into from
Dec 21, 2021
Merged

ref(dsn): trim dsn size #4325

merged 15 commits into from
Dec 21, 2021

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Dec 17, 2021

Removed the class constructor and refactored the utility fn to allow for better tree shaking. Instead of using the Dsn constructor, you should now use makeDsn and dsnToString equiv to the Dsn.toString() class method that we used before

@github-actions
Copy link
Contributor

github-actions bot commented Dec 17, 2021

size-limit report

Path Base Size (1c3f85a) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 20.31 KB 20.25 KB -0.28% 🔽
@sentry/browser - CDN Bundle (minified) 64.7 KB 64.53 KB -0.26% 🔽
@sentry/browser - Webpack 22.29 KB 22.24 KB -0.23% 🔽
@sentry/browser - Webpack - gzip = false 76.23 KB 75.97 KB -0.35% 🔽
@sentry/react - Webpack 22.33 KB 22.27 KB -0.23% 🔽
@sentry/nextjs Client - Webpack 46.45 KB 46.39 KB -0.12% 🔽
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.45 KB 28.39 KB -0.22% 🔽

@JonasBa JonasBa force-pushed the jb/ref/dsn branch 5 times, most recently from 0dcc288 to 61feeeb Compare December 17, 2021 10:49
@mitsuhiko
Copy link
Member

I believe we cannot do this change as we support browsers that do not have the URL object.

@JonasBa
Copy link
Member Author

JonasBa commented Dec 17, 2021

@mitsuhiko I reverted the commits that changed parsing, there should probably still be a small decrease in bundle size

packages/utils/src/dsn.ts Outdated Show resolved Hide resolved
}
validateDsn(components);

const dsn: Dsn = {
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge fan of returning objects with toString methods directly on it, rather than prototypes. Do we need toString at all? Can we instead make it a helper function?

Copy link
Member

Choose a reason for hiding this comment

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

I also noticed a test failure, i think the toString function in it is the culprit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mitsuhiko, not a fan either, I didnt want to do too many changes, but since we should be the only ones to consume this, let's try remove it. btw, I already made it a helper fn - the toString that we expose just calls dsntoString 😅


/** The Sentry Dsn, identifying a Sentry instance and project. */
export class Dsn implements DsnComponents {
Copy link
Member

Choose a reason for hiding this comment

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

Mind writing a quick summary on how to convert from using the Dsn class to the functions? We’ll need to add it to the changelog for react native and electron.

@JonasBa JonasBa force-pushed the jb/ref/dsn branch 2 times, most recently from db2aa7d to a22532a Compare December 21, 2021 07:51
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.

3 participants