feat: Migrate Logger to TypeScript#1055
Conversation
|
| verbose: (msg: string) => void; | ||
| warning: (msg: string) => void; | ||
| error: (msg: string) => void; | ||
| setLogLevel: (msg: string) => void; |
There was a problem hiding this comment.
| setLogLevel: (msg: string) => void; | |
| setLogLevel: (logLevel: string) => void; |
| function Logger(this: Logger, config: SDKInitConfig): void { | ||
| var self = this; | ||
| var logLevel = config.logLevel || 'warning'; | ||
| var logLevel: string = config.logLevel || 'warning'; |
There was a problem hiding this comment.
You should use let and const in this file instead of var.
| this.setLogLevel = function(newLogLevel) { | ||
| this.setLogLevel = function(newLogLevel: string) { | ||
| logLevel = newLogLevel; | ||
| this.logLevel = logLevel; |
There was a problem hiding this comment.
I think you should be able to get rid of the redundant assignment and simplify this:
this.setLogLevel = function(_logLevel: string) {
this.logLevel = _logLevel;
| verbose: (msg: string) => void; | ||
| warning: (msg: string) => void; | ||
| error: (msg: string) => void; | ||
| setLogLevel: (msg: string) => void; |
There was a problem hiding this comment.
Also, you should create a type definition for log level. For example:
type LogLevelType = 'none' | 'verbose' | 'warning' | 'error';
| } | ||
|
|
||
| this.verbose = function(msg) { | ||
| this.verbose = function(msg: string) { |
There was a problem hiding this comment.
Can we try replacing this with arrow functions?
this.verbose = (msg: string) => {
if (logLevel === 'verbose') {
this.logger.verbose?.(msg);
}
};
| }; | ||
|
|
||
| this.warning = function(msg) { | ||
| this.warning = function(msg: string) { |
There was a problem hiding this comment.
Can we trying using arrow function here too?
this.warning = (msg: string) => {
if (logLevel !== 'none') {
if (
this.logger.warning &&
(logLevel === 'verbose' || logLevel === 'warning')
) {
this.logger.warning(msg);
}
}
};
|
Question: Curious why are you targeting |
rmi22186
force-pushed
the
development
branch
5 times, most recently
from
905641c to
b16bdd2
Compare
|
Since #1120 was merged, this can be closed, right @gtamanaha ? |
Yes, lets close this one. |
5 participants
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)