Consolidating ClientService and js-ws and js-rpc integration

[tegefaulkes]

Description

In this PR we're consolidating the client service logic into a ClientService class that handles its life cycle.

Issues Fixed

• Fixes Consolidating client service logic and lifecycle #559

Tasks

• 1. Create a ClientService class within the client domain.
• 2. Move RPC and WebsocketServer creation and life-cycle into ClientService.
• 3. PolykeyAgent will be updated to use this.
• 4. Tests will be updated to use this. Should reduce setup logic.
• 5. Add PolykeyAgent.clientServiceHost, PolykeyAgent.clientServicePort, PolykeyAgent.agentServiceHost, PolykeyAgent.agentServicePort as getter property so that they can be used directly instead of referring to encapsulated objects for the host and port.

Final checklist

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

[ghost]

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

Legend

[CMCDragonkai]

I'm reassigning this to @amydevs and @addievo. You can take over this PR or join this PR for integrating js-ws and js-rpc.

Make sure to do a release before integrating here.

This PR could be rebased on top of #552 if #552 merges first.

If this merges first, then #552 can be rebased on top.

[CMCDragonkai]

Remember to review #559.

However if this is consolidating, @tegefaulkes how come there isn't any deleted files in the diff right? I thought files would be moved from somewhere else.

[tegefaulkes]

Remember to review #559.

However if this is consolidating, @tegefaulkes how come there isn't any deleted files in the diff right? I thought files would be moved from somewhere else.

No deletions yet, it's mostly consolidating the client into a ClientService that handles all of the client stuff. So it was just adding that and modifying other places to use it. No deletions needed there.

However if we're using the js-rpc and js-ws libraries now then we need to remove the rpc and websocket domain and replace it with the libs.

[CMCDragonkai]

[CMCDragonkai]

@addievo add yourself to the constributors in package.json.

[CMCDragonkai]

@amydevs make sure to push often here even for intermediate commits.

[amydevs]

Client service will require an empty stream handler for the ClientServiceClient, as this behaviour is currently unexpected and will cause creation of unneeded resources. When we decide to add push notifs, etc. We can replace this with handlers for that.

[CMCDragonkai]

Just immediately close the stream as soon as you get the event. For now server shouldn't be creating streams. But we will make use of this feature later in the future.

[CMCDragonkai]

Has this been rebased on #560?

[CMCDragonkai]

This is the main PR to work on now.

Goals:

• Get js-ws and js-rpc fully integrated and tested
• Fix up any tests that relate to using the client service and/or agent service
• Make sure this is working with Polykey-CLI

I can see that there are branch conflicts in this PR... I thought this was rebased?

[CMCDragonkai]

@amydevs please rebase this on top of staging too.

[tegefaulkes]

It was rebased, but a linting commit in staging brought in a bunch of conflicts. They'll be mostly whitespace but git doesn't fully understand that.

[CMCDragonkai]

It occurs to me that the serverManifest is a parameter of the NodeConnectionManager but the clientManifest the NodeConnection uses is hard coded. This may be something we'd want to make consistent.

Make them both hardcoded or both paramaterised but defaulted?

[tegefaulkes]

Easiest way is to bind this back for the generator functions.

[CMCDragonkai]

Did you make both clientManifest and serverManifest hardcoded?

[CMCDragonkai]

Easiest way is to bind this back for the generator functions.

I'm not sure this makes sense to me. I just asked chatgpt...

The constructor binding approach generally won't work if you define the async generator function as a class field, like handle = async *function () { }. Class fields are essentially syntactic sugar that allows you to declare and initialize instance properties directly within the class body. When you define a function as a class field, it gets attached to the instance, not the prototype. Therefore, binding it again inside the constructor doesn't work as expected, because the class field has already captured the instance context.

Here's an example to demonstrate:

class MyClass {
  constructor() {
    this.handle = this.handle.bind(this);  // This line is essentially a no-op
  }

  handle = async function*() {
    // `this` is already bound to the class instance because of the class field initialization
  }
}

In this case, the handle function is already bound to the instance of the class due to the class field definition. The constructor's explicit bind() becomes a no-op in this context because handle is already an instance method, not a prototype method. Therefore, binding it again in the constructor doesn't change the behavior of this within handle.

So, if you're using class fields to define your async generator function, you don't need to worry about binding this in the constructor—it's automatically bound to the instance of the class.

[CMCDragonkai]

On second thought... chatgpt might be wrong here.

You are correct, and I apologize for the earlier error. The constructor binding is indeed effective when using class field syntax with async function* or regular functions. It's not a no-op in this case; it would bind this to the class instance.

When you define a method as a class field like handle = async function*() { }, it doesn't automatically bind this. The method is initialized with each new instance of the class, but its this context is determined by how it's invoked, just like any regular function.

Binding the function in the constructor would ensure that this is correctly set to the instance of the class. Here's an example:

class MyClass {
  constructor() {
    this.handle = this.handle.bind(this);
  }

  handle = async function*() {
    // `this` is now correctly bound to the class instance
  }
}

In this example, explicitly binding this.handle in the constructor ensures that it will always be bound to an instance of MyClass, regardless of how or where it is invoked. Thank you for bringing this to my attention, and I appreciate the opportunity to correct this information.

[tegefaulkes]

I fixed that by doing the binding when registering the handler inside the RPCServer. Pretty much how it worked before but I only changed it back for the generators.

[tegefaulkes]

ClientService still needs to be converted to StartStop rather than CreateDestroyStartStop. I'll make an issue in a moment.

[tegefaulkes]

I'm skipping squashing here. This has been merged into and I don't want any potential problems.

[tegefaulkes]

Did you make both clientManifest and serverManifest hardcoded?

The clientManifest is hard coded inside the NodeConnection. The serverManifest is passed into the NCM prevent it from depending on everything during testing.

[tegefaulkes]

New issue #565.