Skip to content

feat(browser): simplify wrap/fill #4286

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

Merged
merged 11 commits into from
Dec 16, 2021
Merged

feat(browser): simplify wrap/fill #4286

merged 11 commits into from
Dec 16, 2021

Conversation

mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Dec 14, 2021

This is a small experiment what happens if we do not define __sentry__ on our wrappers. It tries to reuse some code and removes handleEvent which is a dead code path in practice. I would prefer if i could also remove the wrap() calls on the args. As far as I can see this is all undocumented behavior and it does not look like we depend on this.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2021

size-limit report

Path Base Size (fb87d34) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 21.17 KB 21.14 KB -0.14% 🔽
@sentry/browser - CDN Bundle (minified) 67.7 KB 67.44 KB -0.38% 🔽
@sentry/browser - Webpack 22.79 KB 22.78 KB -0.08% 🔽
@sentry/browser - Webpack - gzip = false 78.22 KB 78.11 KB -0.15% 🔽
@sentry/react - Webpack 22.83 KB 22.81 KB -0.08% 🔽
@sentry/nextjs Client - Webpack 47.33 KB 47.33 KB +0.01% 🔺
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.36 KB 29.34 KB -0.08% 🔽

@AbhiPrasad AbhiPrasad added this to the Treeshaking / Bundle Size milestone Dec 14, 2021
@mitsuhiko mitsuhiko changed the title experiment: slightly different wraps feat(browser): simplify wrap/fill Dec 15, 2021
@mitsuhiko mitsuhiko marked this pull request as ready for review December 15, 2021 12:14
@mitsuhiko
Copy link
Contributor Author

I cannot find a real world situation where the handleEvent branch is taken. There is a test for it, but it's based on a handleEvent on a function which does not appear to be a thing in reality. I believe the code might have predated the manual patching of handleEvent in the addEventHandler instrumentation. I verified that this still works.

__sentry__ itself is definitely optional and also causing issues in #4239 so taking this out seems sensible to me.

@mitsuhiko mitsuhiko merged commit 940faf8 into master Dec 16, 2021
@mitsuhiko mitsuhiko deleted the feature/other-wrap branch December 16, 2021 00:22
onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
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