-
Notifications
You must be signed in to change notification settings - Fork 0
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
Change the LogSink interface to take an optional Context object #18
Conversation
FYI, I will wait for review on this. (No rush, I know that there are a lot of things happening) |
src/logger.ts
Outdated
console[level]( | ||
...this._format(level, ...stringified(context).concat(args)), | ||
); | ||
} |
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.
console[level]( | |
...this._format(level, ...stringified(context).concat(args)), | |
); | |
} | |
console[level]( | |
...this._format( | |
level, | |
...stringified(context), | |
...args), | |
); | |
} |
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.
Excellent, thank you!
src/logger.ts
Outdated
log(level: LogLevel, context: Context | undefined, ...args: unknown[]): void { | ||
console[level]( | ||
logLevelPrefix[level], | ||
...stringified(context).concat(args).map(normalizeArgument), |
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 do not think you need to call normalizeArgument
on the stringified context
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.
Good point, then I can unroll this one too.
src/logger.ts
Outdated
|
||
constructor(level: LogLevel = 'info', logSink: LogSink = consoleLogSink) { | ||
super(logSink, level); | ||
private readonly _context?: Context; |
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.
no optional
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.
Done.
src/logger.test.ts
Outdated
lc3.debug?.('f'); | ||
expect(mockDebug.lastCall.args).to.deep.equal(['d=e', 'f']); | ||
}); | ||
|
||
class TestLogSink implements LogSink { | ||
messages: [LogLevel, ...unknown[]][] = []; | ||
// Note: Order in messages is different from log args order to verify |
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.
You could also have used:
messages: [LogLevel, Context | undefined, unknown[]][] = [];
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.
Love it. Now my verification code reads as it should:
['info', {abc: 'is', easy: 'as'}, [1, 2, 3]],
Thanks!
src/logger.test.ts
Outdated
[undefined, 'info', 1, 2], | ||
[{foo: {bar: 'baz'}}, 'debug', 3, 4], | ||
[{boo: 'oof'}, 'info', 5, 6], | ||
[{abc: 'is', easy: 'as'}, 'info', 1, 2, 3], |
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.
❤️
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 |
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.
Thank you @arv!
src/logger.test.ts
Outdated
lc3.debug?.('f'); | ||
expect(mockDebug.lastCall.args).to.deep.equal(['d=e', 'f']); | ||
}); | ||
|
||
class TestLogSink implements LogSink { | ||
messages: [LogLevel, ...unknown[]][] = []; | ||
// Note: Order in messages is different from log args order to verify |
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.
Love it. Now my verification code reads as it should:
['info', {abc: 'is', easy: 'as'}, [1, 2, 3]],
Thanks!
src/logger.test.ts
Outdated
[undefined, 'info', 1, 2], | ||
[{foo: {bar: 'baz'}}, 'debug', 3, 4], | ||
[{boo: 'oof'}, 'info', 5, 6], | ||
[{abc: 'is', easy: 'as'}, 'info', 1, 2, 3], |
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.
😁
src/logger.ts
Outdated
console[level]( | ||
...this._format(level, ...stringified(context).concat(args)), | ||
); | ||
} |
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.
Excellent, thank you!
src/logger.ts
Outdated
log(level: LogLevel, context: Context | undefined, ...args: unknown[]): void { | ||
console[level]( | ||
logLevelPrefix[level], | ||
...stringified(context).concat(args).map(normalizeArgument), |
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.
Good point, then I can unroll this one too.
src/logger.ts
Outdated
|
||
constructor(level: LogLevel = 'info', logSink: LogSink = consoleLogSink) { | ||
super(logSink, level); | ||
private readonly _context?: Context; |
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.
Done.
Logger-side changes to support https://github.com/rocicorp/mono/issues/191
Also rename
LogContext.addContext()
toLogContext.withContext()
to better convey the immutability of the API.Note: breaking API change