Details

Reviewers

varun
bartek
atul
ashoat

Commits

rCOMMfe02aef634ed: [lib] Basic implementation of identity search context provider without message…

Summary

This implements a basic context provider which connects to the identity search service

Depends on D10928

Test Plan

flow and verified connection by writing some basic code on web utilizing context provider

Diff Detail

Repository

rCOMM Comm

Branch

wyilio/prefix-client

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

will created this revision.Feb 2 2024, 8:39 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 2 2024, 8:39 PM

will retitled this revision from [lib] Basic implementation of identity search context provider without message sending to [lib] [2/n] Basic implementation of identity search context provider without message sending.Feb 2 2024, 8:40 PM

will edited the summary of this revision. (Show Details)

will added a parent revision: D10928: [lib] Add types for identity search websocket server.

Remove lib/identity-search/identity-search.js

Harbormaster completed remote builds in B26475: Diff 36599.Feb 2 2024, 8:58 PM

Harbormaster completed remote builds in B26476: Diff 36600.

will requested review of this revision.Feb 2 2024, 9:00 PM

ashoat added inline comments.Feb 3 2024, 9:28 AM

lib/identity-search/identity-search-context.js
64	Was this accidentally left over?
68	Should `onclose` be handled too?
105	I guess these `console.log`s get replaced in later diffs?

will planned changes to this revision.Feb 5 2024, 7:42 AM

will retitled this revision from [lib] [2/n] Basic implementation of identity search context provider without message sending to [lib] [3/n] Basic implementation of identity search context provider without message sending.Feb 5 2024, 7:45 AM

addressed feedback and rebased to use modified flow types

will added inline comments.Feb 5 2024, 11:00 AM

lib/identity-search/identity-search-context.js
68	Yeah. Addressed in latest diff
105	Besides for the auth token console log which I just removed, Tunnelbroker also keeps these console logs.

Can you explain which of these console.logs are going to get removed in a later diff, versus which ones are meant to stay long-term?

All the console logs currently in the latest diff (36632) I plan to keep long-term.

Harbormaster completed remote builds in B26500: Diff 36632.Feb 5 2024, 11:17 AM

will planned changes to this revision.Feb 5 2024, 11:50 AM

Let's strip the logs for things we expect to happen

We should also consider the dev environment. If we expect the calls to fail in the dev failure, then we should find a way to silence the logs. Guessing it should work fine with staging though, right?

lib/identity-search/identity-search-context.js
69 ↗	(On Diff #36632)	I don't think we should keep this
109 ↗	(On Diff #36632)	I don't think we should keep this

will added a child revision: D10973: [lib] Check for previous auth message and websocket connection state in identity search context provider.Feb 6 2024, 12:41 PM

will retitled this revision from [lib] [3/n] Basic implementation of identity search context provider without message sending to [lib] [5/n] Basic implementation of identity search context provider without message sending.Feb 6 2024, 11:39 PM

will retitled this revision from [lib] [5/n] Basic implementation of identity search context provider without message sending to [lib] Basic implementation of identity search context provider without message sending.Feb 8 2024, 10:14 AM

Remove expected logs

Harbormaster failed remote builds in B26665: Diff 36875!Feb 8 2024, 11:04 AM

rebase

Harbormaster completed remote builds in B26777: Diff 37017.Feb 13 2024, 12:48 AM

will added a reviewer: ashoat.Feb 13 2024, 10:55 AM

ashoat accepted this revision.Feb 13 2024, 11:18 AM

ashoat added inline comments.

lib/identity-search/identity-search-context.js
110 ↗	(On Diff #37017)	Please make sure all lines are no longer than 80 chars (except where forced by Prettier)
115 ↗	(On Diff #37017)	You should add a space after the colon here
123 ↗	(On Diff #37017)	Shouldn't this be `identitySearchMessageToServerTypes`?
150 ↗	(On Diff #37017)	I assume you'll need to add a method later to enable a consumer of this context to send messages (ie. search queries) across the socket?

This revision is now accepted and ready to land.Feb 13 2024, 11:18 AM

ashoat added inline comments.Feb 13 2024, 11:19 AM

lib/identity-search/identity-search-context.js
115 ↗	(On Diff #37017)	Actually not sure about this... how does `console.log` for `native` / `web` print two params? Does it already put some whitespace between them? If no, please add a space in all of the relevant spots here. If yes, you can ignore this feedback

varun added inline comments.Feb 13 2024, 11:25 AM

lib/identity-search/identity-search-context.js
115 ↗	(On Diff #37017)	`console.log` automatically adds whitespace when there are multiple params

will added inline comments.Feb 13 2024, 2:39 PM

lib/identity-search/identity-search-context.js
150 ↗	(On Diff #37017)	Yep! This happens in https://phab.comm.dev/D11000 which was already reviewed

will added inline comments.Feb 13 2024, 2:41 PM

lib/identity-search/identity-search-context.js
123 ↗	(On Diff #37017)	Yeah. Thanks for catching this

address feedback. Shorten line size and use server heartbeat type

Harbormaster completed remote builds in B26806: Diff 37049.Feb 13 2024, 3:05 PM

[lib] rebase with identity search prefix and add auth message hook

will requested review of this revision.Feb 21 2024, 1:20 PM

will added a reviewer: atul.

Implementing the useGetIdentitySearchAuthMessage follows discussion from https://phab.comm.dev/D11059

will mentioned this in D11059: [native] Utilize identity search on native.Feb 21 2024, 1:28 PM

Harbormaster completed remote builds in B27046: Diff 37415.Feb 21 2024, 1:35 PM

Already reviewed this one – looks like @will wants a review from @atul based on history

lib/utils/identity-search-utils.js
12 ↗	(On Diff #37415)	You don't need the question mark here

atul accepted this revision.Feb 22 2024, 7:52 PM

atul added inline comments.

lib/identity-search/identity-search-context.js
75 ↗	(On Diff #37415)	Do we need any sort of validation here on the client side? Guessing there's validation on the identity service

This revision is now accepted and ready to land.Feb 22 2024, 7:52 PM

will added inline comments.Feb 24 2024, 12:34 AM

lib/identity-search/identity-search-context.js
75 ↗	(On Diff #37415)	Yeah. It's mainly validation on the identity service which then sends a response back whether or not the authMessage was valid or not.