Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

Add missing types, fix remaining faulty ones #67

Merged
merged 4 commits into from
Nov 8, 2020
Merged

Conversation

rekmarks
Copy link
Member

I missed a couple of errors in #66. While fixing them, I decided to add types for all exports as well.

@rekmarks rekmarks requested a review from a team as a code owner October 26, 2020 04:19
export type JsonRpcEngineNextCallback = (
returnFlightCallback?: (done: () => void) => void,
returnHandlerCallback?: ReturnHandlerCallback,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of the errors. You can actually pass errors to the return handler callbacks in order to cause the JSON-RPC request to return an error.

src/index.d.ts Outdated
Comment on lines 101 to 103
type ScaffoldMiddlewareHandler = JsonRpcMiddleware | (
boolean | number | string | Record<string, unknown> | unknown[] | null | undefined
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little iffy. Really, I want to type it like: if it's a function, then it has to extend JsonRpcMiddleware, otherwise it can be anything.

OTOH, even that is perhaps a bit too permissive, since the values should probably be serializable 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this? I think what you have is correct but just throwing out another possibility to look into.

type Serializable = boolean | number | string | Record<string, unknown> | unknown[] | null | undefined;
type ScaffoldMiddlewareHandler<T> = T extends Function ? JsonRpcMiddleware : Serializable;

export interface createScaffoldMiddleware<T> {
  (handlers: {[methodName: string]: ScaffoldMiddlewareHandler<T>}): JsonRpcMiddleware;
}

brad-decker
brad-decker previously approved these changes Nov 3, 2020
Copy link

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, added a thing you could tinker with but its non blocking

src/index.d.ts Outdated
Comment on lines 101 to 103
type ScaffoldMiddlewareHandler = JsonRpcMiddleware | (
boolean | number | string | Record<string, unknown> | unknown[] | null | undefined
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this? I think what you have is correct but just throwing out another possibility to look into.

type Serializable = boolean | number | string | Record<string, unknown> | unknown[] | null | undefined;
type ScaffoldMiddlewareHandler<T> = T extends Function ? JsonRpcMiddleware : Serializable;

export interface createScaffoldMiddleware<T> {
  (handlers: {[methodName: string]: ScaffoldMiddlewareHandler<T>}): JsonRpcMiddleware;
}

@rekmarks
Copy link
Member Author

rekmarks commented Nov 8, 2020

@brad-decker, that's a really neat suggestion! I accepted it here: 51f7f9a

Copy link

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot LGTM

@rekmarks rekmarks merged commit 1e6f647 into master Nov 8, 2020
@rekmarks rekmarks deleted the more-type-fixups branch November 8, 2020 03:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants