-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref: Make it easier to use stackParser #5015
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.
I think that's a good change. Some tests still need adjustments small import adjustments like here:
import { defaultStackParsers, opera10StackParser, opera11StackParser } from '../../../src/stack-parsers'; | |
const operaParser = createStackParser(opera10StackParser, opera11StackParser); | |
const chromiumParser = createStackParser(...defaultStackParsers); |
How about: import { defaultBrowserOptions } from '@sentry/browser';
const client = BrowserClient({
...defaultBrowserOptions
}); import { defaultBrowserOptions } from '@sentry/browser';
const client = BrowserClient({
...defaultBrowserOptions,
transport: myCustomTransportFactory
}); import { defaultBrowserOptions } from '@sentry/browser';
const { transport, stackParser, integrations } = defaultBrowserOptions;
const client = BrowserClient({
transport,
stackParser,
integrations
}); or call it |
@kamilogorek do you mean Seems clean but would default integrations then still get tree-shaken out if they're not used? Say in a case like this: import { defaultBrowserOptions } from '@sentry/browser';
const client = BrowserClient({
...defaultBrowserOptions,
integrations: []
}); |
Yes I'm not sure, but there's a chance they would not, it'd have to be tested.
has a higher chance of being tree shaken (at least from a logical perspective) |
We had a chat about this - decided against the |
1869bb7
to
5fab95a
Compare
5fab95a
to
100ba1a
Compare
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.
LGTM, thanks for fixing the tests and for adding the migration changes!
Co-authored-by: Luca Forstner <[email protected]>
After writing up #5011, I realized using clients manually was kind of annoying as you had to use a util to pass in the
stackParser
correctly:This PR update exports so that you can just reference the
defaultStackParser
directly.