-
-
Notifications
You must be signed in to change notification settings - Fork 62
Conversation
"engines": { | ||
"node": ">=10.0.0" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript complains without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to limit the amount of feedback on changes that were unrelated to this work, but I may have left one or two. I tagged feedback according to https://www.notion.so/Code-Review-ebbc0a971d4c4ed6845db1a280ea1910
stack?: unknown; | ||
} | ||
|
||
export interface JsonRpcRequest<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?: Some interfaces are declared with generics but only appear in the code being instantiated with <unknown>
. This isn't my only question related to this, but I suppose I am wondering what the intention was of exposing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intend for consumers to import these types and e.g. use them in their middlewares and to cast results from engine.handle
.
In addition to addressing feedback, I made some other changes: |
_originalError?: unknown; | ||
} | ||
|
||
type InternalMiddleware = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use a Generic, but not worth doing if we will remove these eventually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and we don't perform type-sensitive operations on response.result
or response.error
internally anyway.
}); | ||
} | ||
|
||
private async _processRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could carry the generic all the way down the chain into these private methods, so long as you don't attempt to narrow this type they'll be treated as unknown but won't cause any heartache for end-users when they use the generics to define the params/response types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might do that in the follow-up when I remove the internal types!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @rekmarks 🎉
A lot of other packages depend on
json-rpc-engine
, and it's high time we migrate it to TypeScript. The implementation is unchanged, but this is breaking because some stuff has moved around:src/index.ts
/dist/index.js
asMiddleware
is converted to an instance method ofJsonRpcEngine