Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix: switch to user-provided logger rather than console.log #3466

Merged
merged 4 commits into from
Aug 5, 2022
Merged

fix: switch to user-provided logger rather than console.log #3466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5511e63
use user-provided logger in blockchain.resume
MicaiahReid Jul 28, 2022
d114934
use user-provided logger in ws-handler
MicaiahReid Aug 2, 2022
a91ae0e
remove unused dependency
MicaiahReid Aug 3, 2022
bb4234c
pass logging instead of all options
MicaiahReid Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/chains/ethereum/ethereum/src/blockchain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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:" 🦖

this.#options.logging.logger.log(
"Warning: startMining called when miner was already started"
);
return;
}

Expand Down
18 changes: 12 additions & 6 deletions src/chains/ethereum/ethereum/src/forking/handlers/ws-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { EthereumInternalOptions } from "@ganache/ethereum-options";
import { AbortError, CodedError } from "@ganache/ethereum-utils";
import { AbortError } from "@ganache/ethereum-utils";
import { AbortSignal } from "abort-controller";
import WebSocket from "ws";
import { Handler } from "../types";
Expand All @@ -23,7 +23,10 @@ export class WsHandler extends BaseHandler implements Handler {
constructor(options: EthereumInternalOptions, abortSignal: AbortSignal) {
super(options, abortSignal);

const { url, origin } = options.fork;
const {
fork: { url, origin },
logging
} = options;

this.connection = new WebSocket(url.toString(), {
origin,
Expand All @@ -40,11 +43,11 @@ export class WsHandler extends BaseHandler implements Handler {
// handler too.
this.connection.binaryType = "nodebuffer";

this.open = this.connect(this.connection);
this.open = this.connect(this.connection, logging);
this.connection.onclose = () => {
// try to connect again...
// TODO: backoff and eventually fail
this.open = this.connect(this.connection);
this.open = this.connect(this.connection, logging);
};
this.abortSignal.addEventListener("abort", () => {
this.connection.onclose = null;
Expand Down Expand Up @@ -98,7 +101,10 @@ export class WsHandler extends BaseHandler implements Handler {
}
}

private connect(connection: WebSocket) {
private connect(
connection: WebSocket,
logging: EthereumInternalOptions["logging"]
) {
let open = new Promise((resolve, reject) => {
connection.onopen = resolve;
connection.onerror = reject;
Expand All @@ -109,7 +115,7 @@ export class WsHandler extends BaseHandler implements Handler {
connection.onerror = null;
},
err => {
console.log(err);
logging.logger.log(err);
}
);
return open;
Expand Down