This PR name is too specific, make sure to consider all of the issue scope.

Ready for review

Rebase on staging.

And it's ready for review... not ready for merge.

Rebased on staging

Open up a co-PR on PK that assumes this is being integrated. I need to see the whole context.

@CMCDragonkai I need more context about the spec before I can do a full review.

I recall discussing that we'd move the metadata out of the params and response fields and merge it into the JSONRPC message structure as optional parameters. Is that still a requirement?

For example, the JSONRPCRequestMessage becomes something like this?

// old
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
};

// new
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
  metadata?: {
    timeout?: number | null;
  } & Omit<Record<string, JSONValue>, 'timeout'>;
};

@CMCDragonkai I need more context about the spec before I can do a full review.

I recall discussing that we'd move the metadata out of the params and response fields and merge it into the JSONRPC message structure as optional parameters. Is that still a requirement?

For example, the JSONRPCRequestMessage becomes something like this?

// old
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
};

// new
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
  metadata?: {
    timeout?: number | null;
  } & Omit<Record<string, JSONValue>, 'timeout'>;
};

What's the trade off here? Is it just that we leave the json RPC spec? Maybe it's necessary.

I think it's needed. It's a hard requirement that we can't impose structure on the params and response of the messages. These are purely application defined. So if we have default timeout middleware that requires a timeout parameter in the message, it can't be part of the application supplied data.

But this has other consequences, if metadata is no longer supplied as part of the input and output of the caller and handler, then how does the caller and handler access or set the metadata now?

If we DO impose structure on the params and result then they can't be JSONValue anymore. It needs to be a Record<string, JSONValue> and we need to enforce types on the metadata and required parameters while preventing the application from overwriting their types. That may be easier to implement since there will be less changes to the API. But if we're using the RPC from the 3rd party perspective, the structure of the params will need to be know and that has usability issues since we're still diverging from the JSONRPC spec.

In either case there is more work to be done here.

Based on discussion, we're going to expand on the params and result parameter types to include the metadata and timeout. This has the least required changes to make this work.

we need to modify the JSONRPC message types to reflect this.

I'd have to fully review the changes to know what's still needed. But the following needs to change.

1. JSONRPC messages are updated so the at the RPCRequestParams<T> is used for the params type, and the RPCResponseResult<T> is used for the result type. The T is now T extends Record<string, JSONValue> = ObjectEmpty.
2. Tests need to be updated to reflect this.
3. Need to check that any arbitrary metadata can be provided, but the timeout type can't be overwritten.

This will come after #47, and @amydevs can take over this to update the JSON RPC types, and factor out the timeout middleware.

Should be ready to review, still need to update description, cos i pushed a few unrelated fixes regarding another issue

@amydevs tasks on OP spec need to be covered.

@amydevs can you also explain what also needs to be done on PK as well to support this change? What will need to be removed. Is there an existing issue in MatrixAI/Polykey-CLI#40?