feat: JsonRpcEngineV2
#6176
feat: JsonRpcEngineV2
#6176
Conversation
rekmarks
force-pushed
the
rekm/json-rpc-engine-rewrite-next
branch
from
f74dff2 to
c393838
Compare
There was a problem hiding this comment.
Since TypeScript is capable of inferring types from runtime function parameter inputs at the call site, would it make sense to infer the type signatures for the JsonRpcEngineV2 class methods instead of assigning type constraints, which are by design wider than necessary?
The intention of the following comments is to narrow the methods down to literal types, while preserving all of the type constraints in the original code, and without imposing any additional restrictions or causing any breaking changes to the class's current API. (The newly added generic type arguments do not need to be manually supplied and can be omitted at the call site).
This could provide some additional flexibility and potentially valuable type information in downstream contexts where the class methods are called.
(identical changes in diff format:)
diff --git a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
index 7ca5ec220..9755abbe5 100644
--- a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
+++ b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
@@ -97,10 +97,10 @@ export class JsonRpcEngineV2<
* @param options.context - The context to pass to the middleware.
* @returns The JSON-RPC response.
*/
- async handle(
- request: Request & JsonRpcRequest,
- options?: HandleOptions,
- ): Promise<Result>;
+ async handle<
+ InputRequest extends Request & JsonRpcRequest,
+ InputOptions extends HandleOptions,
+ >(request: InputRequest, options?: InputOptions): Promise<Result>;
/**
* Handle a JSON-RPC notification. No result will be returned.
@@ -109,14 +109,17 @@ export class JsonRpcEngineV2<
* @param options - The options for the handle operation.
* @param options.context - The context to pass to the middleware.
*/
- async handle(
- notification: Request & JsonRpcNotification,
- options?: HandleOptions,
- ): Promise<void>;
-
- async handle(
- request: Request,
- { context }: HandleOptions = {},
+ async handle<
+ Notification extends Request & JsonRpcNotification,
+ InputOptions extends HandleOptions,
+ >(notification: Notification, options?: InputOptions): Promise<void>;
+
+ async handle<
+ InputRequest extends Request,
+ InputOptions extends HandleOptions,
+ >(
+ request: InputRequest,
+ { context }: InputOptions = {} as InputOptions,
): Promise<Result | void> {
const isReq = isRequest(request);
const { result } = await this.#handle(request, context);
@@ -139,10 +142,10 @@ export class JsonRpcEngineV2<
* @param options.context - The context to pass to the middleware.
* @returns The JSON-RPC response, if any.
*/
- async handleAny(
- request: JsonRpcCall & Request,
- options?: HandleOptions,
- ): Promise<Result | void> {
+ async handleAny<
+ InputRequest extends JsonRpcCall & Request,
+ InputOptions extends HandleOptions,
+ >(request: InputRequest, options?: InputOptions): Promise<Result | void> {
return this.handle(request, options);
}
@@ -154,9 +157,12 @@ export class JsonRpcEngineV2<
* @param context - The context to pass to the middleware.
* @returns The result from the middleware.
*/
- async #handle(
- originalRequest: Request,
- context: MiddlewareContext = new MiddlewareContext(),
+ async #handle<
+ InputRequest extends Request,
+ InputContext extends MiddlewareContext,
+ >(
+ originalRequest: InputRequest,
+ context: InputContext = new MiddlewareContext() as InputContext,
): Promise<{
result: Result | void;
request: Readonly<Request>;
@@ -195,10 +201,14 @@ export class JsonRpcEngineV2<
* @param context - The context to pass to the middleware.
* @returns The `next()` function factory.
*/
- #makeNextFactory(
- middlewareIterator: Iterator<JsonRpcMiddleware<Request, Result | void>>,
- state: RequestState<Request, Result>,
- context: MiddlewareContext,
+ #makeNextFactory<
+ InputIterator extends Iterator<JsonRpcMiddleware<Request, Result | void>>,
+ CurrentState extends RequestState<Request, Result>,
+ InputContext extends MiddlewareContext,
+ >(
+ middlewareIterator: InputIterator,
+ state: CurrentState,
+ context: InputContext,
): () => Next<Request, Result> {
const makeNext = (): Next<Request, Result> => {
let wasCalled = false;
@@ -245,10 +255,10 @@ export class JsonRpcEngineV2<
* @param result - The result from the middleware.
* @param state - The current values of the request and result.
*/
- #updateResult(
- result: Result | void,
- state: RequestState<Request, Result>,
- ): void {
+ #updateResult<
+ InputResult extends Result | void,
+ CurrentState extends RequestState<Request, Result>,
+ >(result: InputResult, state: CurrentState): void {
if (isNotification(state.request) && result !== undefined) {
throw new JsonRpcEngineError(
`Result returned for notification: ${stringify(state.request)}`,
@@ -269,7 +279,10 @@ export class JsonRpcEngineV2<
* @param currentRequest - The current request.
* @param nextRequest - The next request.
*/
- #assertValidNextRequest(currentRequest: Request, nextRequest: Request): void {
+ #assertValidNextRequest<
+ CurrentRequest extends Request,
+ NextRequest extends Request,
+ >(currentRequest: CurrentRequest, nextRequest: NextRequest): void {
if (nextRequest.jsonrpc !== currentRequest.jsonrpc) {
throw new JsonRpcEngineError(
`Middleware attempted to modify readonly property "jsonrpc" for request: ${stringify(currentRequest)}`,| async handle( | ||
| request: Request & JsonRpcRequest, | ||
| options?: HandleOptions, | ||
| ): Promise<Result>; |
There was a problem hiding this comment.
| async handle( | |
| request: Request & JsonRpcRequest, | |
| options?: HandleOptions, | |
| ): Promise<Result>; | |
| async handle< | |
| InputRequest extends Request & JsonRpcRequest, | |
| InputOptions extends HandleOptions, | |
| >(request: InputRequest, options?: InputOptions): Promise<Result>; |
| async handle( | ||
| notification: Request & JsonRpcNotification, | ||
| options?: HandleOptions, | ||
| ): Promise<void>; |
There was a problem hiding this comment.
| async handle( | |
| notification: Request & JsonRpcNotification, | |
| options?: HandleOptions, | |
| ): Promise<void>; | |
| async handle< | |
| InputNotification extends Request & JsonRpcNotification, | |
| InputOptions extends HandleOptions, | |
| >(notification: InputNotification, options?: InputOptions): Promise<void>; |
| async handle( | ||
| request: Request, | ||
| { context }: HandleOptions = {}, |
There was a problem hiding this comment.
| async handle( | |
| request: Request, | |
| { context }: HandleOptions = {}, | |
| async handle< | |
| InputRequest extends Request, | |
| InputOptions extends HandleOptions, | |
| >( | |
| request: InputRequest, | |
| { context }: InputOptions = {} as InputOptions, |
| async handleAny( | ||
| request: JsonRpcCall & Request, | ||
| options?: HandleOptions, |
There was a problem hiding this comment.
| async handleAny( | |
| request: JsonRpcCall & Request, | |
| options?: HandleOptions, | |
| async handleAny< | |
| InputRequest extends JsonRpcCall & Request, | |
| InputOptions extends HandleOptions, | |
| >( | |
| request: InputRequest, | |
| options?: InputOptions, |
| async #handle( | ||
| originalRequest: Request, | ||
| context: MiddlewareContext = new MiddlewareContext(), |
There was a problem hiding this comment.
| async #handle( | |
| originalRequest: Request, | |
| context: MiddlewareContext = new MiddlewareContext(), | |
| async #handle< | |
| InputRequest extends Request, | |
| InputContext extends MiddlewareContext, | |
| >( | |
| originalRequest: InputRequest, | |
| context: InputContext = new MiddlewareContext() as InputContext, |
| #makeNextFactory( | ||
| middlewareIterator: Iterator<JsonRpcMiddleware<Request, Result | void>>, | ||
| state: RequestState<Request, Result>, | ||
| context: MiddlewareContext, |
There was a problem hiding this comment.
| #makeNextFactory( | |
| middlewareIterator: Iterator<JsonRpcMiddleware<Request, Result | void>>, | |
| state: RequestState<Request, Result>, | |
| context: MiddlewareContext, | |
| #makeNextFactory< | |
| InputIterator extends Iterator<JsonRpcMiddleware<Request, Result | void>>, | |
| CurrentState extends RequestState<Request, Result>, | |
| InputContext extends MiddlewareContext, | |
| >( | |
| middlewareIterator: InputIterator, | |
| state: CurrentState, | |
| context: InputContext, |
| #updateResult( | ||
| result: Result | void, | ||
| state: RequestState<Request, Result>, | ||
| ): void { |
There was a problem hiding this comment.
| #updateResult( | |
| result: Result | void, | |
| state: RequestState<Request, Result>, | |
| ): void { | |
| #updateResult< | |
| InputResult extends Result | void, | |
| CurrentState extends RequestState<Request, Result>, | |
| >(result: InputResult, state: CurrentState): void { |
| * @param currentRequest - The current request. | ||
| * @param nextRequest - The next request. | ||
| */ | ||
| #assertValidNextRequest(currentRequest: Request, nextRequest: Request): void { |
There was a problem hiding this comment.
| #assertValidNextRequest(currentRequest: Request, nextRequest: Request): void { | |
| #assertValidNextRequest< | |
| CurrentRequest extends Request, | |
| NextRequest extends Request, | |
| >(currentRequest: CurrentRequest, nextRequest: NextRequest): void { |
There was a problem hiding this comment.
Some opportunities to leverage type inference and satisfies for narrower return types to JsonRpcEngineV2 class methods:
(identical changes in diff format:)
diff --git a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
index 7ca5ec220..9c6d2c336 100644
--- a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
+++ b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts
@@ -139,11 +139,8 @@ export class JsonRpcEngineV2<
* @param options.context - The context to pass to the middleware.
* @returns The JSON-RPC response, if any.
*/
- async handleAny(
- request: JsonRpcCall & Request,
- options?: HandleOptions,
- ): Promise<Result | void> {
- return this.handle(request, options);
+ async handleAny(request: JsonRpcCall & Request, options?: HandleOptions) {
+ return this.handle(request, options) satisfies Promise<Result | void>;
}
/**
@@ -157,18 +154,15 @@ export class JsonRpcEngineV2<
async #handle(
originalRequest: Request,
context: MiddlewareContext = new MiddlewareContext(),
- ): Promise<{
- result: Result | void;
- request: Readonly<Request>;
- }> {
+ ) {
this.#assertIsNotDestroyed();
deepFreeze(originalRequest);
- const state: RequestState<Request, Result> = {
+ const state = {
request: originalRequest,
result: undefined,
- };
+ } as const satisfies RequestState<Request, Result>;
const middlewareIterator = this.#makeMiddlewareIterator();
const firstMiddleware = middlewareIterator.next().value;
@@ -181,7 +175,10 @@ export class JsonRpcEngineV2<
});
this.#updateResult(result, state);
- return state;
+ return state satisfies {
+ result: Result | void;
+ request: Readonly<Request>;
+ };
}
/**
@@ -200,7 +197,7 @@ export class JsonRpcEngineV2<
state: RequestState<Request, Result>,
context: MiddlewareContext,
): () => Next<Request, Result> {
- const makeNext = (): Next<Request, Result> => {
+ const makeNext = () => {
let wasCalled = false;
const next = async (
@@ -235,7 +232,7 @@ export class JsonRpcEngineV2<
return next;
};
- return makeNext;
+ return makeNext satisfies () => Next<Request, Result>;
}
/**
@@ -292,16 +289,15 @@ export class JsonRpcEngineV2<
*
* @returns The JSON-RPC middleware.
*/
- asMiddleware(): JsonRpcMiddleware<Request, Result> {
+ asMiddleware() {
this.#assertIsNotDestroyed();
-
- return async ({ request, context, next }) => {
+ return (async ({ request, context, next }) => {
const { result, request: finalRequest } = await this.#handle(
request,
context,
);
return result === undefined ? await next(finalRequest) : result;
- };
+ }) satisfies JsonRpcMiddleware<Request, Result>;
}
/**| ): Promise<Result | void> { | ||
| return this.handle(request, options); |
There was a problem hiding this comment.
| ): Promise<Result | void> { | |
| return this.handle(request, options); | |
| ) { | |
| return this.handle(request, options) satisfies Promise<Result | void>; |
There was a problem hiding this comment.
This and some other suggestions would violate @typescript-eslint/explicit-function-return-type, which is currently disabled for this monorepo, but there's a TODO to enable it. I understand it to be part of our convention. Is the only way to benefit from satisfies to remove the return annotation?
There was a problem hiding this comment.
Agreed. We definitely want to enable that rule at some point and I would be wary of creating work for ourselves.
@MajorLift Why is using satisfies preferred over specifying a return type?
There was a problem hiding this comment.
Explicit return type annotations overwrite narrower literal types that are inferred at the call site, resulting in a loss of type information. satisfies is a strictly superior alternative, as it also enforces an upper bound for type safety, but doesn't prevent type inference.
Is the only way to benefit from
satisfiesto remove the return annotation?
Unfortunately, yes. Because of this, I think we should explicitly ban @typescript-eslint/explicit-function-return-type. I'm drafting an entry in our contributor-docs that will reverse the current recommendation to use explicit return annotations for functions.
RE readability concerns, the jsdoc @returns tag can be used for any information that needs to be included in-line or available via intellisense. This also appears to be a strictly superior alternative to return type annotations.
There was a problem hiding this comment.
Here are some examples of how the following three options widen or narrow function return types at the call site:
- Explicit return type annotation
- Return type inferred (optionally with
satisfiesconstraint) - Generic parameter type inference (optionally with
satisfiesconstraint)
/**
* Example 1: Identity Function
*/
const widened = identityFunctionWithReturnTypeAnnotation(1) // Json
const preserved = identityFunctionWithReturnTypeInferred(1) // number
const narrowed = identityFunctionWithGenericParameterTypeInferred(1) // 1
function identityFunctionWithReturnTypeAnnotation(num: number): Json {
return num
}
function identityFunctionWithReturnTypeInferred(num: number) {
return num satisfies Json
}
function identityFunctionWithGenericParameterTypeInferred<Num extends number>(num: Num) {
return num satisfies Json
}
/**
* Example 2: Join Strings
*/
const widened2 = joinStringsWithReturnTypeAnnotation('hello', 'world') // string
const preserved2 = joinStringsWithReturnTypeInferred('hello', 'world') // `${string} ${string}`
const narrowed2 = joinStringsWithGenericParameterTypesInferred('hello', 'world') // "hello world"
function joinStringsWithReturnTypeAnnotation(head: string, tail: string): string {
return `${head} ${tail}` as const
}
function joinStringsWithReturnTypeInferred(head: string, tail: string) {
return `${head} ${tail}` as const satisfies string
}
function joinStringsWithGenericParameterTypesInferred<Head extends string, Tail extends string>(head: Head, tail: Tail) {
return `${head} ${tail}` as const satisfies string
}| ): Promise<{ | ||
| result: Result | void; | ||
| request: Readonly<Request>; | ||
| }> { |
There was a problem hiding this comment.
| ): Promise<{ | |
| result: Result | void; | |
| request: Readonly<Request>; | |
| }> { | |
| ) { |
| const state: RequestState<Request, Result> = { | ||
| request: originalRequest, | ||
| result: undefined, | ||
| }; |
There was a problem hiding this comment.
| const state: RequestState<Request, Result> = { | |
| request: originalRequest, | |
| result: undefined, | |
| }; | |
| const state = { | |
| request: originalRequest, | |
| result: undefined, | |
| } as const satisfies RequestState<Request, Result>; |
| }); | ||
| this.#updateResult(result, state); | ||
|
|
||
| return state; |
There was a problem hiding this comment.
| return state; | |
| return state satisfies { | |
| result: Result | void; | |
| request: Readonly<Request>; | |
| }; |
| state: RequestState<Request, Result>, | ||
| context: MiddlewareContext, | ||
| ): () => Next<Request, Result> { | ||
| const makeNext = (): Next<Request, Result> => { |
There was a problem hiding this comment.
| const makeNext = (): Next<Request, Result> => { | |
| const makeNext = () => { |
| return next; | ||
| }; | ||
|
|
||
| return makeNext; |
There was a problem hiding this comment.
| return makeNext; | |
| return makeNext satisfies () => Next<Request, Result>; |
| asMiddleware(): JsonRpcMiddleware<Request, Result> { | ||
| this.#assertIsNotDestroyed(); | ||
|
|
||
| return async ({ request, context, next }) => { | ||
| const { result, request: finalRequest } = await this.#handle( | ||
| request, | ||
| context, | ||
| ); | ||
| return result === undefined ? await next(finalRequest) : result; | ||
| }; |
There was a problem hiding this comment.
| asMiddleware(): JsonRpcMiddleware<Request, Result> { | |
| this.#assertIsNotDestroyed(); | |
| return async ({ request, context, next }) => { | |
| const { result, request: finalRequest } = await this.#handle( | |
| request, | |
| context, | |
| ); | |
| return result === undefined ? await next(finalRequest) : result; | |
| }; | |
| asMiddleware() { | |
| this.#assertIsNotDestroyed(); | |
| return (async ({ request, context, next }) => { | |
| const { result, request: finalRequest } = await this.#handle( | |
| request, | |
| context, | |
| ); | |
| return result === undefined ? await next(finalRequest) : result; | |
| }) satisfies JsonRpcMiddleware<Request, Result>; |
There was a problem hiding this comment.
nit: Narrower types for JsonRpcServer['handle']:
diff --git a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
index 9283c9349..c5968c64f 100644
--- a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
+++ b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
@@ -90,7 +90,7 @@ export class JsonRpcServer {
* @returns The JSON-RPC response, or `undefined` if the request is a
* notification.
*/
- async handle(rawRequest: unknown): Promise<JsonRpcResponse | undefined> {
+ async handle(rawRequest: unknown) {
const [originalId, isRequest] = getOriginalId(rawRequest);
try {
@@ -100,10 +100,10 @@ export class JsonRpcServer {
if (isRequest) {
return {
jsonrpc,
- id: originalId as JsonRpcId,
+ id: originalId,
// The result is guaranteed to be Json by the engine.
- result: result as Json,
- };
+ result: result as unknown as Json,
+ } as const satisfies JsonRpcResponse;
}
} catch (error) {
this.#handleError?.(error);
@@ -113,12 +113,12 @@ export class JsonRpcServer {
jsonrpc,
// Remap the original id to the error response, regardless of its
// type, which is not our problem.
- id: originalId as JsonRpcId,
+ id: originalId,
error: serializeError(error, {
shouldIncludeStack: false,
shouldPreserveMessage: true,
}),
- };
+ } as const satisfies JsonRpcResponse;
}
}
return undefined;
@@ -196,8 +196,14 @@ function hasValidParams(
* @returns The original id and a boolean indicating if the request is a request
* (as opposed to a notification).
*/
-function getOriginalId(rawRequest: unknown): [unknown, boolean] {
- if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) {
+function getOriginalId<RawRequest extends Pick<JsonRpcRequest, 'id'> | unknown>(
+ rawRequest: RawRequest,
+): [JsonRpcId, true] | [undefined, false] {
+ if (
+ isObject(rawRequest) &&
+ // @ts-expect-error Force `hasProperty` to accept non-objects as input.
+ hasProperty<Pick<JsonRpcRequest, 'id'>, 'id'>(rawRequest, 'id')
+ ) {
return [rawRequest.id, true];
}| function getOriginalId(rawRequest: unknown): [unknown, boolean] { | ||
| if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) { |
There was a problem hiding this comment.
| function getOriginalId(rawRequest: unknown): [unknown, boolean] { | |
| if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) { | |
| function getOriginalId<RawRequest extends Pick<JsonRpcRequest, 'id'> | unknown>( | |
| rawRequest: RawRequest, | |
| ): [JsonRpcId, true] | [undefined, false] { | |
| if ( | |
| isObject(rawRequest) && | |
| // @ts-expect-error Force `hasProperty` to accept non-objects as input. | |
| hasProperty<Pick<JsonRpcRequest, 'id'>, 'id'>(rawRequest, 'id') | |
| ) { |
There was a problem hiding this comment.
The types in this suggestion are incorrect; the original id can be malformed. See #6176 (comment)
There was a problem hiding this comment.
Unless I'm mistaken, I believe the rest of the suggestions in this review don't work without this change.
There was a problem hiding this comment.
Would this better align with the expected shape of rawRequest?
| function getOriginalId(rawRequest: unknown): [unknown, boolean] { | |
| if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) { | |
| function getOriginalId< | |
| RawRequest extends (Omit<JsonRpcRequest, 'id'> & { id: unknown }) | unknown, | |
| >(rawRequest: RawRequest): [unknown, true] | [undefined, false] { | |
| if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) { |
There was a problem hiding this comment.
The above would enable the return type annotation on handle to be replaced with satisfies:
diff --git a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
index b467e59a8..05a5d38ad 100644
--- a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
+++ b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts
@@ -90,7 +90,7 @@ export class JsonRpcServer {
* @returns The JSON-RPC response, or `undefined` if the request is a
* notification.
*/
- async handle(rawRequest: unknown): Promise<JsonRpcResponse | undefined> {
+ async handle(rawRequest: unknown) {
// If rawRequest is not a notification, the originalId will be attached
// to the response. We attach our own, trusted id in #coerceRequest()
// while the request is being handled.
@@ -105,7 +105,7 @@ export class JsonRpcServer {
jsonrpc,
id: originalId as JsonRpcId,
result,
- };
+ } as const satisfies JsonRpcResponse;
}
} catch (error) {
this.#onError?.(error);
@@ -120,7 +120,7 @@ export class JsonRpcServer {
shouldIncludeStack: false,
shouldPreserveMessage: true,
}),
- };
+ } as const satisfies JsonRpcResponse;
}
}
return undefined;
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * `engine`. | ||
| */ | ||
| constructor(options: Options) { | ||
| this.#handleError = options.handleError; |
There was a problem hiding this comment.
What do you think about renaming this to onError?
What do you think about inheriting from EventEmitter?
There was a problem hiding this comment.
onError is good, I'll do that.
What's the use case for inheriting from EventEmitter?
There was a problem hiding this comment.
My thought was that if there are other events we want to emit in the future we can use a well-established interface to do so. We've used EventEmitter in the past, certainly.
But maybe onError is simpler anyhow, and it wouldn't require inheritance to implement.
There was a problem hiding this comment.
I see. Based on us not needing them before now, I don't anticipate a need for other events. We can always extend SafeEventEmitter later if the spirit moves us.
| Request extends JsonRpcCall = JsonRpcCall, | ||
| Result extends Json = Json, |
There was a problem hiding this comment.
Why are Request and Result type parameters of the entire engine? Each middleware in the engine can take different things and return different things, and each request itself will have different parameters and different results. All requests and results need to match a certain supertype, of course, so maybe that's what this represents? Still it seems a bit strange.
| Result extends Json = Json, | ||
| > { | ||
| #middleware: Readonly< | ||
| NonEmptyArray<JsonRpcMiddleware<Request, Result | void>> |
There was a problem hiding this comment.
Continuing the previous comment, is it really possible to narrow the request and result for all middleware in this way? It seems to me that the best that we can say that is that a middleware takes a request or a notification and returns a JSON-compliant result, i.e., the type here should be JsonRpcMiddleware<JsonRpcRequest<unknown>, unknown>. Or does this PR introduce a new constraint such that all middleware in an engine must behave the same way?
There was a problem hiding this comment.
This PR does not introduce any new constraints. It is possible to narrow the types for all middleware, e.g. you could have an engine where all the middleware expect notifications with number params, or one where they only expect string params. As a practical matter, I don't expect this functionality to be used very frequently, and we'll likely default to pretty broad generic types. I suppose I could see "forking" our JSON-RPC pipeline on requests vs. notifications as one possible use.
Although in truth, I'm kind of agnostic about this, and would be happy to ship an engine and server without generic parameters. I cribbed the generics from an earlier rewrite by @Gudahtt, who might have a stronger case to make for the generics?
There was a problem hiding this comment.
is it really possible to narrow the request and result for all middleware in this way?
It should be 🤔 Is there a reason this wouldn't be possible?
Narrowing a middleware to "notifications only" or "requests only" is certainly a good use case. But it seems possible that we could be more specific than that, tracking precisely which types of messages each engine is meant to handle.
I haven't experimented with this though.
There was a problem hiding this comment.
The generics would allow for creating a more constrained "sub-engine" at some point in a middleware stack—again requests vs notifications come to mind—and unless we think keeping the generics, I suppose I don't see a strong case for getting rid of them.
| * @param options.context - The context to pass to the middleware. | ||
| * @returns The JSON-RPC response, if any. | ||
| */ | ||
| async handleAny( |
There was a problem hiding this comment.
If we have a method that can accept either requests and notifications, should we now have a method that accepts only requests and another method that accepts only notifications? In other words, in addition to handleAny (or something), have handleRequests and handleNotifications. This way we could avoid needing to define overloads altogether.
There was a problem hiding this comment.
Diving into this more deeply, can you provide more details on why you added this? I understand that we don't want to mess with overloads here but the existence of this method also implies that there are some cases where we want a combined return type and I am not understanding that.
There was a problem hiding this comment.
@Gudahtt was also irked by the existence of this method. We tried and failed to get rid of it. handleAny is purely an artifact of a problem with the types. Namely, if you pass .handle() a JsonRpcCall, the result is always typed as void. See for example:
async function test() {
const engine = new JsonRpcEngineV2({
middleware: [
() => null,
],
});
const r1 = await engine.handle({
jsonrpc: '2.0',
method: 'test',
} as JsonRpcCall)
const r2 = await engine.handle({
id: 1,
jsonrpc: '2.0',
method: 'test',
} as JsonRpcCall)
}
In this example, r1 and r2 are both void.
I will update the docstring with an explanation of this.
There was a problem hiding this comment.
IMO, separate entrypoints for notifications and requests would be a cure worse than the disease. In that case, we'd put the caller in the position of figuring out whether something is a request or notification before passing it to the engine, which is properly the job of the engine (or JsonRpcServer).
@MajorLift I'm going to hold off on these changes until we settle #6176 (comment) |
|
@MajorLift It's true that using Maybe we could make an exception in cases where the improved types are especially valuable? But generally it seems like a good guideline to me. Readability is important. |
|
@Gudahtt @MajorLift if this satisfies everyone's concerns at this time, I will review @MajorLift's suggestions and implement any that seem like material improvements to the API (private and public). Otherwise I'll keep the types as-is and we can discuss our conventions at the lint config or some other appropriate venue. |
@MajorLift regarding this suggestion, I tried it out but I don't believe that the added flexibility / type information will be useful. Since it complicates the method signatures, it does not seem worth it in this case. |
|
My concern is that the current method signatures force all components in a given middleware chain to be defined (as opposed to just constrained) with the same
It does appear that the V2 engine is effectively enforcing a type-level constraint that all middleware in a given chain either share the same type signature, or at least be polymorphic over the same
Would the above still be true if some middleware in a given chain needs to be typed with params/results that are different from the others? In that case, inferred type parameters could be a way to reduce complexity and confusion surrounding types at the call site, even if it might sometimes come at the cost of more complicated type signatures at the definition. Alternatively, the status quo of consistently typing all middleware and methods using the widest constraint type could work in most cases -- as long as the lost type information isn't valuable, and it's made clear to the users that any narrower types that are inferred at the call site should be asserted/overwritten with the constraint types (this second point should be documented to minimize confusion). I guess it comes down to how practically relevant this concern is. If the intent is to uniformly type (i.e. prevent independent polymorphic typing of) all middleware in any given middleware chain, then I agree that switching to inferred type parameters and Edit: On reflection, it might be fair to say that this typing issue is out-of-scope for the intention of this PR. |
Explanation
Introduces
JsonRpcEngineV2andJsonRpcServer, intended to replace all existing usage of the existingJsonRpcEngineimplementation. For the motivation behind this change, see #6088 and/or this ADR.Implementation
In order to resolve the problems listed in the motivation, V2 engine is split in two:
JsonRpcEngineV2JsonRpcServererrorHandlerconstructor parameter for capturing engine / middleware errorsSee the updated package
README.mdfor details.Migration
While this PR is substantial, migrating our existing JSON-RPC pipelines will be a significant project involving multiple teams over many releases cycles. To facilitate this, the PR introduces a forwards-compatibility adapter,
asV2Middleware(), and backwards-compatibility adapter,asLegacyMiddleware(), for the legacy and V2 engines, respectively. In addition, all V2 exports are exposed under the/v2export path, making this update completely non-breaking (although all legacy exports are deprecated).Note to reviewers
I recommend proceeding as follows:
JsonRpcEngineV2JsonRpcServerReferences
JsonRpcEnginefor safety, ergonomics, and debuggability #6088JsonRpcEngineChecklist
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes