Skip to content

PKClient needs to use WebSocketClient and RPCClient in the start/stop procedure#602

Merged
CMCDragonkai merged 4 commits intostagingfrom
feature-pkclient-createdestroy
Oct 24, 2023
Merged

PKClient needs to use WebSocketClient and RPCClient in the start/stop procedure#602
CMCDragonkai merged 4 commits intostagingfrom
feature-pkclient-createdestroy

Conversation

@CMCDragonkai
Copy link
Copy Markdown
Member

@CMCDragonkai CMCDragonkai commented Oct 24, 2023

Description

The PKClient needs to use WS and RPC in the start/stop so that we can start and stop the PK client to any given client service, while also destroying is for destroying the session state.

Issues Fixed

Tasks

  • 1. Moved WebSocketClient and RPCClient into start/stop
  • 2. Exposed PolykeyClient.nodeId
  • 3. PolykeyClient.start now takes a subset of the creation parameters, and you can stop and start a connection to different client service

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

… `try` block so that the info log is at the top
…` in the start/stop procedures, and destroy is kept for destroying session state
@ghost
Copy link
Copy Markdown

ghost commented Oct 24, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@CMCDragonkai
Copy link
Copy Markdown
Member Author

Note that options are never saved since they are always re-defaulted.

So when you create, stop, start. On the start call you have pass back whatever options you wanted.

UX wise it might be unexpected. We could solve this in the future by saving the options on creation, and then having start default to the options set on creation if they exist, otherwise the default config. It would be a good idea to then extend mergeObjects to be multi-arity.

@CMCDragonkai CMCDragonkai merged commit c3413f1 into staging Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant