Enhance error handling to support third-party tools #509
Conversation
- Replaced consoleLogger with the new Logger - Removed jest mock where consoleLogger was used - Improved support for third-party tools
| }); | ||
| if (!c.ok) { | ||
| console.error(c.log); | ||
| logger.error(c.log); | ||
| process.exit(1); |
There was a problem hiding this comment.
My guess would be that all things like process.exit should also go
|
Any progress here? |
| if (message instanceof Error) { | ||
| message = message.stack || message.message; | ||
| } else { | ||
| message = "" + message.toString(); |
There was a problem hiding this comment.
Can we simply use message.toString() here?
| const message = "Bindings compiler crashed"; | ||
| logger.error(message); | ||
| logger.error(e as Error); | ||
| errorMessages.push(new Error(message)); |
There was a problem hiding this comment.
Should we also include e to the errorMessages structure? It might be useful to third-parties, I think.
There was a problem hiding this comment.
Yes, that would be a better.
| const message = "Report generation crashed"; | ||
| logger.error(message); | ||
| logger.error(e as Error); | ||
| errorMessages.push(new Error(message)); |
| }) { | ||
| const configWithRootPath = await loadConfig(args.fileName, args.configPath); | ||
| if (!configWithRootPath) { | ||
| return false; | ||
| return { ok: false, error: [new Error("Unable to load config")] }; |
There was a problem hiding this comment.
nit: Maybe we should add args.configPath to the error message in order to give more context to the user.
There was a problem hiding this comment.
We can change it to something `Unable to load config from path: ${args.configPath}`
|
|
||
| export async function build(args: { | ||
| config: ConfigProject; | ||
| project: VirtualFileSystem; | ||
| stdlib: string | VirtualFileSystem; | ||
| logger?: TactLogger | null | undefined; | ||
| }) { | ||
| logger?: Logger | null | undefined; |
There was a problem hiding this comment.
This type definition looks a bit strange, right? What is the different between null and undefined values? What value will be used if we write if (!logger) { in the function body?
There was a problem hiding this comment.
I think we can remove both null and undefined since it will be undefined by default.
| @@ -111,7 +112,7 @@ export async function build(args: { | |||
| let codeEntrypoint: string; | |||
|
|
|||
| // Compiling contract to func | |||
| logger.log(" > " + contract + ": tact compiler"); | |||
| logger.info(" > " + contract + ": tact compiler"); | |||
There was a problem hiding this comment.
nit: Maybe we should use string interpolation in the similar cases instead. AFAIK we prefer that style when refactoring the project, but it's up to you.
There was a problem hiding this comment.
Yes, I'll do it. I haven't done the refactoring yet, but I'll take care of it.
Closes #262