ref: Avoid using global singleton for logger #8880
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/better-logging
branch
from
99b05ba
to
c629f35
Compare
mydea
force-pushed
the
fn/better-logging
branch
2 times, most recently
from
5345ce6
to
39f404d
Compare
mydea
force-pushed
the
fn/better-logging
branch
from
fb6ca14
to
03025a7
Compare
| expect(consoleMessages).toEqual( | ||
| hasDebug | ||
| ? [ | ||
| 'Sentry Logger [log]: Integration installed: InboundFilters', | ||
| 'Sentry Logger [log]: Integration installed: FunctionToString', | ||
| 'Sentry Logger [log]: Integration installed: TryCatch', | ||
| 'Sentry Logger [log]: Integration installed: Breadcrumbs', | ||
| 'Sentry Logger [log]: Global Handler attached: onerror', | ||
| 'Sentry Logger [log]: Global Handler attached: onunhandledrejection', | ||
| 'Sentry Logger [log]: Integration installed: GlobalHandlers', | ||
| 'Sentry Logger [log]: Integration installed: LinkedErrors', | ||
| 'Sentry Logger [log]: Integration installed: Dedupe', | ||
| 'Sentry Logger [log]: Integration installed: HttpContext', | ||
| 'Sentry Logger [warn]: Discarded session because of missing or non-string release', | ||
| 'test log', | ||
| ] | ||
| : ['[Sentry] Cannot initialize SDK with `debug` option using a non-debug bundle.', 'test log'], |
There was a problem hiding this comment.
l: This test is gonna fail whenever we add/remove a default integration or change the ordering. I guess one can argue in both directions whether that's a good thing or not, given that it even sort of tests that the integration is actually enabled by default. The downside is that this isn't strictly the purpose of this test. To get around this, we could use expect.stringContaining and only expect specific integrations. But in the end this is logaf-l for me so feel free to ignore :)
There was a problem hiding this comment.
Yeah, I thought about the same thing, and was also not 100% sure... IMHO it's OK to leave it like this for now, if we get to the point where we add/change default integrations and this becomes annoying we can still replace it with something more generic. IMHO for now it is not bad to be "notified" if the defaults change somehow (and that rarely happens anyhow...!)
Instead of #8873, this tries to simplify the logger to avoid the global lookup.
If you have more than one utils, having multiple loggers would be the least of your problems. So IMHO no need to special case handle this, in the worst case logging should still work somehow 🤔
Note some test code seems to rely on the global stuff the logger does, which required minor changes there.
This now relies on the
originalConsoleMethodswe keep in the console instrumentation, so no need to look for__sentry_original__anymore. If that doesn't exist, nothing to do for us (because we haven't instrumented the console).Closes #8741