-
Notifications
You must be signed in to change notification settings - Fork 50
chore: remove console logs #262
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
base: main
Are you sure you want to change the base?
chore: remove console logs #262
Conversation
| console.info( | ||
| `Implement onException on ${this.#ParentClass.name} to handle this error.` | ||
| ); | ||
| // console.error( |
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.
we want to keep this one, no? odd to have an exception that you haven't caught or handled here
| console.log( | ||
| `Implement onAlarm on ${this.#ParentClass.name} to handle alarms.` | ||
| ); | ||
| // console.log( |
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.
we want to keep this one, no? would be weird to set an alarm and not have overridden this
| console.warn( | ||
| `onRequest hasn't been implemented on ${this.#ParentClass.name}:${this.name} responding to ${request.url}` | ||
| ); | ||
| // console.warn( |
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.
we want to keep this one, or you'll get a request without a log, and won't know what to do here?
| console.info( | ||
| `Implement onError on ${this.#ParentClass.name} to handle this error.` | ||
| ); | ||
| // console.error( |
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.
same as onException, you probably don't want a websocket error that isn't handled
| console.info( | ||
| `Implement onMessage on ${this.#ParentClass.name} to handle this message.` | ||
| ); | ||
| // console.log( |
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.
receiving messages without handling them?
| console.log( | ||
| `Connection ${connection.id} connected to ${this.#ParentClass.name}:${this.name}` | ||
| ); | ||
| // console.log( |
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.
this one I could be convinced, but it is causing a problem?
|
I think this has something to do with how but yeah, I went over board, in reality I only want the onConnection console to be gone, which should have been overriden by agent core, but look like agent core calls in the original function If you want I will edit the PR to only fix the one console log I was annoyed about so that you can merge it |
|
hmm ok I think there might be a bug with the on* method binding, I'll have a look |
Serverclass has bunch of default console.log which are not overriden by Agent'son*methods, so if you are usingAgentclass that is based onServeryour logs gets filled with the default connected/disconnected messages which is annoyingso I just removed the default console logs as they serve no purpose and most people overwrite them anyways