-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(tracing): update hasTracingEnabled return value #4225
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
Conversation
size-limit report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows how the implementation could be a oneliner...
return options && ('tracesSampleRate' in options || 'tracesSampler' in options);
packages/tracing/test/utils.test.ts
Outdated
const tracesSampler = () => 1; | ||
const tracesSampleRate = 1; | ||
it.each([ | ||
['no options', undefined, false], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the output in this one case depend on the return value of getCurrentHub()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it depends on getCurrentHub
, but there is no client attached to it during the test, so it works as expected.
I agree it could be a one liner, but I didn't change the function to minimize changing behaviour. I can go back and update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
5417ded
to
e4fc16a
Compare
Experimenting with bundle size that didn't make any progress, but cherry-picked this off that branch.