Cleanups #570
Cleanups #570
Conversation
gustavohenke
left a comment
gustavohenke
left a comment
There was a problem hiding this comment.
Would've enjoyed having some time to review this.
Several of my comments are around some of the stylistic changes.
If it's not enforced through linting, then it's not pragmatic to change something that's been on main for a while, as it's bound to cause bikeshedding in future changes/churn.
There was a problem hiding this comment.
🤔 why is this a .d.ts now?
| @@ -1,4 +1,6 @@ | |||
| #!/usr/bin/env node | |||
| import process from 'node:process'; | |||
There was a problem hiding this comment.
are we enforcing the node: import prefix somehow?
| placeholderTarget <= this.additionalArguments.length | ||
| ) { | ||
| return quote([this.additionalArguments[placeholderTarget - 1]]); | ||
| if (+placeholderTarget <= this.additionalArguments.length) { |
There was a problem hiding this comment.
nit: I have a slight preference for Number() over + for casting to number due to it being clearer about the intention
| interface OutgoingMessageEvent extends MessageEvent { | ||
| options?: MessageOptions; | ||
| onSent(error?: unknown): void; | ||
| onSent: (error?: unknown) => void; |
There was a problem hiding this comment.
Looks like we're also preferring properties over methods in interfaces now.
Did you have a reason, and is this enforceable?
| beforeEach(() => { | ||
| controller.handle(commands); | ||
| outputStream.emit('error', new Error()); | ||
| outputStream.emit('error', new Error('test')); |
There was a problem hiding this comment.
Looks like another pattern here - did you want to enforce every error instance having a message?
| BY Fabio Spampinato | ||
| MIT license | ||
| Copied due to the dependency not being compatible with CommonJS |
There was a problem hiding this comment.
looks like this could now be depended upon, instead of copied.
binintopublishConfigto get rid of the following warning: