feat: logger and emit errors #39
Conversation
| } | ||
|
|
||
| export interface Logger { | ||
| error(message: string | Error, error?: Error): void; |
There was a problem hiding this comment.
I would say that all logger functions should define the argument lists as ...args: unknown[].
There was a problem hiding this comment.
I think typing it is ok, since that shows what we will pass to it. Typescript looks at the type, not the interface that defined the type, so if someone passes a compatible logging object, it's all good. With unknown we couldn't communicate to consumers of this lib if the data we pass changes in the future.
There was a problem hiding this comment.
While I agree on properly typing the interfaces whenever it's possible I don't see much sense to do it for logger's functions parameters, where we should be able to pass anything we want (as i.e. console family does it).
There was a problem hiding this comment.
I honestly don't really care about the logger. For all I care we keep it undefined 🤷
.vscode/settings.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
We should remove this file and add it to gitignore.
|
In your opinion should we remove async from start and emit the listening event instead? Like we do for error? Like a normal server does @balloob @Michsior14 |
|
Porque no los dos? |
|
"Normal server" 🤷 Async awaiting a task is absolutely preferred. It means you know the error that is raised is related to you calling that function. An error event can be emitted by any other action that is currently in progress. |
| @@ -2,7 +2,6 @@ node_modules | |||
| package-lock.json | |||
| config.json | |||
| /dist/ | |||
| .vscode/* | |||
There was a problem hiding this comment.
This one ignores .vscode folder ;)
I guess you should just remove the 4 lines bellow.
There was a problem hiding this comment.
@Michsior14 Yep, @balloob said to ignore the settings.json file but atm that's the only one file inside that folder so why not ignore the entire folder?
There was a problem hiding this comment.
Yep, this is why I suggested to remove lines like !.vscode/settings.json. The above comment was added when you removed this line (.vscode/*) ;)
There was a problem hiding this comment.
It's there... I removed it in one commit to be able to remove the folder, then in the next commit I have readded it
There was a problem hiding this comment.
Yep I know, it's just outdated comment now.
BTW. git rm is a easier way to do it ;)
There was a problem hiding this comment.
@Michsior14 Didn't know about git rm command 😆
src/lib/server.ts
Outdated
|
|
||
| export interface Logger { | ||
| error(message: string | Error, error?: Error): void; | ||
| warn(message: string[]): void; |
There was a problem hiding this comment.
why is this one an array?
Fixes #38
Fixes #37