Approach with examples can be found in this comment.
Adding both @tomek and @bartek as blocking reviewers to make sure severity and types are correct
Details
Details
Tested later in the stack
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/shared/dm-ops/process-dm-ops.js | ||
|---|---|---|
| 183 ↗ | (On Diff #48413) | Isn't this also error? |
| lib/components/debug-logs-context.js | ||
|---|---|---|
| 31 ↗ | (On Diff #48413) | Using a Set might be more semantic, because ordering of the types doesn't matter |
| 125–134 ↗ | (On Diff #48413) | Hardcoding here might be a bit surprising for someone using this API. How about adding a new argument and appending the types? |
| lib/shared/dm-ops/process-dm-ops.js | ||
| 183 ↗ | (On Diff #48413) | Yes, it is |
| lib/tunnelbroker/peer-to-peer-message-handler.js | ||
| 139–157 ↗ | (On Diff #48413) | These two probably should be errors |
| native/account/qr-auth/primary-device-qr-auth-context-provider.js | ||
| 195 ↗ | (On Diff #48413) | Should we introduce an additional type for messages from this file? |
Comment Actions
address review
| lib/components/debug-logs-context.js | ||
|---|---|---|
| 31 ↗ | (On Diff #48413) | makes sense |
| 125–134 ↗ | (On Diff #48413) | In that case, there is no need for the useOlmDebugLogs hook, and the developer should use useDebugLogs - the whole point of useOlmDebugLogs was to encapsulate this logic in the hook because it'll be duplicated in all call sites (and to provide better type checking) |
| lib/shared/dm-ops/process-dm-ops.js | ||
| 183 ↗ | (On Diff #48413) | fixed |
| lib/tunnelbroker/peer-to-peer-message-handler.js | ||
| 139–157 ↗ | (On Diff #48413) | good call |
| native/account/qr-auth/primary-device-qr-auth-context-provider.js | ||
| 195 ↗ | (On Diff #48413) | Not sure, those are errors anyway, and also, I think logging here was added just to use the same approach as in different places, but as you can see, we have alerts her,e so we have a distinct way of logging and I am not sure if adding a button in UI for this will be beneficial for DX |