-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
chore(connect): deviceCommands call and commonCall shallow structure #16735
base: develop
Are you sure you want to change the base?
Conversation
mroz22
commented
Jan 30, 2025
•
edited
Loading
edited
- commit 1: this should be little more readable. transport already never throws and returns { success: true|false}-like objects instead. This commit propagates this trend to a higher layer (deviceCommads call and deviceCommands commonCall. deviceCommands.typedCall still throws but adds "cause" arg transport-error if the error originated in the transport layer. pls check @marekrjpolak
- commit 2: todo for @komret
- commit 3: to be debated @marekrjpolak
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
6a456da
to
785bcc5
Compare
throw error; | ||
|
||
if ('isTransportError' in response) { | ||
throw new Error(response.error, { cause: 'transport-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.
Two-parametrical Error
constructor is not supported in es2019
=> build:lib error. Can we change the target to esnext
or something?
// throw error anyway, next call should be resolved properly | ||
throw error; | ||
|
||
if ('isTransportError' in response) { |
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.
Just to be safe?
if ('isTransportError' in response) { | |
if ('isTransportError' in response && response.isTransportError) { |
): Promise< | ||
DefaultPayloadMessage | (ErrorGeneric<ReadWriteError> & { isTransportError: true }) | ||
> { |
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.
I'm not a big fan of somehow inconsistent return types.
return { | ||
...res, | ||
isTransportError: true, | ||
}; |
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.
Given that _commonCall
is not throwing any more in this case, it will change the logic below in _filterCommonTypes
, e.g. this._commonCall('PinMatrixAck', { pin }).then(response => ...)
, right? Do we mind?
// res.message is not propagated to higher levels, only logged here. webusb/node-bridge may return message with additional information | ||
res.message, | ||
); | ||
throw new Error(res.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.
Maybe I'm missing the point, but why don't we just do the throw new Error(res.error, { cause: 'transport-error' })
thing here, instead of all the other changes? It would propagate upwards anyway, right?
f85d1b7
to
0b7aa27
Compare
fa0a122
to
403f0d0
Compare