Page MenuHomePhabricator

[lib] Modify `getClientResponsesSelector` to return `Promise`
ClosedPublic

Authored by atul on Mar 10 2023, 4:49 PM.
Tags
None
Referenced Files
F3578586: D7034.diff
Sun, Dec 29, 12:46 AM
F3575746: D7034.diff
Sat, Dec 28, 8:38 PM
Unknown Object (File)
Fri, Dec 6, 9:46 PM
Unknown Object (File)
Fri, Dec 6, 6:02 AM
Unknown Object (File)
Fri, Dec 6, 6:02 AM
Unknown Object (File)
Fri, Dec 6, 6:02 AM
Unknown Object (File)
Fri, Dec 6, 6:02 AM
Unknown Object (File)
Nov 6 2024, 3:36 PM
Subscribers
None

Details

Summary

In order to get the signedIdentityKeysBlob, we're going to need to await commCoreModule.getUserPublicKey. As a first step, we refactor getClientResponsesSelector so we'll be able to await getUserPublicKey inside.

Test Plan

Set breakpoints and ensure that native/web send responses as expected and keyserver continues to get responses as expected. Will also be tested implicitly by subsequent diffs.

Haven't done this yet, will test before landing.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Mar 10 2023, 4:57 PM
ashoat added inline comments.
lib/selectors/socket-selectors.js
167

The change to getSignedIdentityKeysBlob doesn't appear to be in this diff. This is "okay" since await doesn't do anything if passed a non-promise IIRC, but I still don't think we should be awaiting here if getSignedIdentityKeysBlob doesn't return a Promise

lib/socket/request-response-handler.react.js
74

Maybe we should just pass serverRequests in here tbh

This revision is now accepted and ready to land.Mar 11 2023, 5:25 AM
lib/selectors/socket-selectors.js
167

getSignedIdentityKeysBlob(...) actually remains completely unimplemented as of this diff. However, the "contract" or interface or whatever of getSignedIdentityKeysBlob() does change in this diff to return a Promise so I think having this await is fine?

lib/socket/request-response-handler.react.js
74

I'm open to either way. Going to land as-is, but will create a Linear task and handle in followup: https://linear.app/comm/issue/ENG-3300/consider-handling-getclientresponses-within

Haven't done this yet, will test before landing.

Set breakpoint in processClientResponses(...) and ensured that clientResponses were as expected:

d18e14.png (988×4 px, 528 KB)

This revision was landed with ongoing or failed builds.Mar 11 2023, 12:29 PM
This revision was automatically updated to reflect the committed changes.