Implement js-sdk-logging in optimizely-sdk #232
Conversation
| } | ||
| config = config || {}; | ||
|
|
||
| // TODO warn about setting per instance errorHandler / logger / logLevel |
There was a problem hiding this comment.
Should we just do this now?
There was a problem hiding this comment.
Yeah I can do that.
|
|
||
| /** | ||
| * Default logger implementation | ||
| */ | ||
| function NoOpLogger() {} |
There was a problem hiding this comment.
Is NoOpLogger needed anymore?
There was a problem hiding this comment.
I kept it around. The preferred way of setting no-op logging is to just call optimizelySDK.setLogger(null).
But people are using NoOpLogger today, so we need to keep it in.
| { | ||
| clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE, | ||
| // always get the OptimizelyLogger facade from logging | ||
| logger: logger, |
There was a problem hiding this comment.
We are still passing a logger into Optimizely. I thought the plan was to stop passing logger around and instead import the singleton wherever necessary. Is that still true/would we implement this in follow-up PRs?
There was a problem hiding this comment.
I left the internal logger passing in place since it reduces the risk of this breaking anything.
As we rewrite things we should be using the singleton. I didn't see any benefit in doing that now, best case scenario things work exactly as before, worse case scenario the SDK breaks.
Add docstrings
Add webpack bundle analyzer
For now just use the prefix via sprintf (what we currently already do)
jordangarcia
force-pushed
the
jordan/implement-core-logger
branch
from
d54d901 to
bedf9a2
Compare
| } else { | ||
| var simpleLogger = logger.createLogger({logLevel: 4}); | ||
| simpleLogger.log(enums.LOG_LEVEL.ERROR, sprintf('%s: %s', MODULE_NAME, ex.message)); | ||
| console.error(ex.message); |
There was a problem hiding this comment.
Sorry, I missed this earlier. But should we be referring to console outside of the console log handler? I get that we want to emit something even when no logger is specified, but maybe we can just add a console log handler temporarily?
There was a problem hiding this comment.
I didn't quite see the point in creating a ConsoleLogHandler just to call it once. I feel like we shouldn't be using the LogHandlers now and only interacting with the Facade.
The result of this whole thing was just to console.error something. So why not just console.error something directly.
There was a problem hiding this comment.
Sounds good, let's merge it.
Summary
Test plan
3.0.1optimizely-sdkand this one to verify they work the same.