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(hub): Remove _invokeClient #4195

Closed
wants to merge 10 commits into from
Closed

ref(hub): Remove _invokeClient #4195

wants to merge 10 commits into from

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Nov 30, 2021

This PR removes the dynamic dispatch of _invokeClient in favour of explicitly using client methods. We introduce a _withClient helper that does this.

_withClient grabs the client and scope from the stack of scopes on the hub, and executes a callback with those values. This allows the consumer of the function to directly access the client instance and call methods on it.

There are a couple consequences of this change

  • Things are now typed with typescript!
  • We remove arg spread operator usage
  • No more string usage from dynamic dispatch

Although the savings are low here (around 0.7 kb bundle improved), it also helps us see how we can address refactoring callOnHub.

It's important to note that this is a breaking change as we are removing the _callOnClient public method exported from @sentry/minimal, so I'm going to apply a blocked label on this until we get consensus. One thing to note here is that is is both @hidden and prefixed with _, so it clearly seems like an internal SDK api. In addition, it seems that with Electron v3, there are no external consumers of the _callOnClient function (no internal consumers either)

@AbhiPrasad AbhiPrasad changed the title Abhi invoke client ref(hub): Remove _invokeClient Nov 30, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 30, 2021

size-limit report

Path Base Size (cabb53c) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 22.48 KB 22.46 KB -0.11% 🔽
@sentry/browser - Webpack 23.37 KB 23.33 KB -0.14% 🔽
@sentry/react - Webpack 23.4 KB 23.37 KB -0.14% 🔽
@sentry/nextjs Client - Webpack 49.73 KB 49.69 KB -0.07% 🔽
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.93 KB 29.9 KB -0.1% 🔽

@AbhiPrasad AbhiPrasad marked this pull request as ready for review November 30, 2021 18:28
@AbhiPrasad AbhiPrasad added this to the Treeshaking / Bundle Size milestone Nov 30, 2021
@AbhiPrasad AbhiPrasad closed this Dec 3, 2021
@AbhiPrasad
Copy link
Member Author

We are closing this PR as the bundle savings are low, and it doesn't necessarily justify the impact from removing a method from @sentry/minimal. In addition, we will be refactoring this when we work on the next major release anyway.

@AbhiPrasad AbhiPrasad deleted the abhi-invoke-client branch December 3, 2021 15:49
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

LGTM.

CHANGELOG.md Show resolved Hide resolved
packages/hub/test/hub.test.ts Show resolved Hide resolved
@lobsterkatie
Copy link
Member

Heh. You beat me to it. I agree that this is a good pattern for the other indirection changes we want to make, so let's bookmark it and we can come back to it then.

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