-
Notifications
You must be signed in to change notification settings - Fork 683
fix: switch to user-provided logger rather than console.log
#3466
Conversation
@@ -618,7 +618,9 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> { | |||
|
|||
resume(_threads: number = 1) { | |||
if (!this.#isPaused()) { | |||
console.log("Warning: startMining called when miner was already started"); |
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.
In other places we console.warn()
.
should we consider extending the logger type to add some sort of log level awareness?
export type Logger = {
log(message?: any, ...optionalParams: any[]): void;
info(message?: any, ...optionalParams: any[]): void;
warn(message?: any, ...optionalParams: any[]): void;
error(message?: any, ...optionalParams: any[]): void;
};
When normalizing we could default info/warn/error back to log to avoid this being a breaking change.
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 it would be great to have a more robust logging system to do this, and I believe it's on the docket (though I couldn't find an issue for it). This PR was meant to just be a quick fix though, so I'd rather get this through. Especially because I hardly consider this a "warning" - there are no consequences to calling miner_start
when the miner isn't paused, so what are we warning them against?
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.
Although... it does start with the word "Warning:" 🦖
@@ -98,7 +98,7 @@ export class WsHandler extends BaseHandler implements Handler { | |||
} | |||
} | |||
|
|||
private connect(connection: WebSocket) { | |||
private connect(connection: WebSocket, options: EthereumInternalOptions) { |
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.
Can we require just the LoggingConfig
here rather than the entire config object?
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'm glad you have an opinion because I kept going back and forth on this. Fixed.
Though the types here are a bit confusing - I think I got it right with EthereumInternalOptions["logging"]
rather than LoggingConfig
.
The Ganache options allow you to set a custom logger to handle any logging that Ganache does*. When using Ganache programatically, this can be done as follows:
However, there were still a few hold-out
console.log
s in our code, which logged directly to the console rather than the user-provided log function. This change fixes this issue.*Pro Tip: You can actually update that
logger
function while Ganache is running to temporarily change how you handle logs, if you're into that sort of thing.