Page MenuHomePhabricator

[lib] introduce Tunnelbroker Context
ClosedPublic

Authored by kamil on Oct 26 2023, 8:50 AM.
Tags
None
Referenced Files
F2898021: D9606.diff
Sat, Oct 5, 1:23 AM
F2895637: D9606.diff
Fri, Oct 4, 6:43 PM
Unknown Object (File)
Wed, Oct 2, 1:07 PM
Unknown Object (File)
Wed, Oct 2, 1:07 PM
Unknown Object (File)
Wed, Oct 2, 1:07 PM
Unknown Object (File)
Wed, Oct 2, 6:01 AM
Unknown Object (File)
Sat, Sep 7, 1:50 AM
Unknown Object (File)
Aug 31 2024, 7:40 AM
Subscribers

Details

Summary

Adding some boilerplate.

Create a separate diff to potentially discuss using a Tunnelbroker connection via context.

Why this solution?

  1. It allows messages to be sent from any component in the tree.
  2. It enables the addition of listeners from any component in the tree.
  3. It facilitates dispatching actions as reactions to received messages without requiring any hacks.
  4. It avoids bloating redux with messages (as in the case with the keyserver socket).

Depends on D9596

Test Plan

N/A

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Oct 27 2023, 5:32 AM

N/A

Can we find a way of testing this code?

lib/tunnelbroker/tunnelbroker-context.js
38–40 ↗(On Diff #32428)

It is just a placeholder, so it doesn't matter, but it could probably be simplified

73 ↗(On Diff #32428)

Is this really necessary for a value that is exported?

This revision is now accepted and ready to land.Oct 30 2023, 4:24 AM
lib/tunnelbroker/tunnelbroker-context.js
38–40 ↗(On Diff #32428)

This is different... @kamil's code returns a promise that never resolves, whereas your code returns a promise that resolves immediately. Not sure what the intention is

lib/tunnelbroker/tunnelbroker-context.js
14 ↗(On Diff #32428)

I think that's more general

18 ↗(On Diff #32428)

Maybe we could implement a pattern where this function returns a cleanup function (that calls removeListener)? Not really needed but might make some code using this nicer.

32 ↗(On Diff #32428)

Is this needed?

address review

lib/tunnelbroker/tunnelbroker-context.js
18 ↗(On Diff #32428)

valid point, but I don't really like to introduce code that will not have usage right now, we can easily add this when needed

32 ↗(On Diff #32428)

it's not, thanks

38–40 ↗(On Diff #32428)

I just needed to put something here because I wanted to split this work into logical diff, actual implementation is here: D9616

73 ↗(On Diff #32428)

looks like it's not, thanks

This revision was automatically updated to reflect the committed changes.