types: Make Data type-parameter optional in JsonRpcError #102
Conversation
legobeat
force-pushed
the
optional-error-data
branch
from
20b7920
to
aa42de1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Update: Made the error data optional in more places; MetaMask/json-rpc-engine#161 passes using this. |
legobeat
force-pushed
the
optional-error-data
branch
from
145961d
to
34049f8
Compare
src/errors.ts
Outdated
| @@ -65,7 +68,7 @@ export const rpcErrors = { | |||
| * @param arg - The error message or options bag. | |||
| * @returns An instance of the {@link JsonRpcError} class. | |||
| */ | |||
| internal: <Data extends DataWithOptionalCause>( | |||
| internal: <Data extends OptionalDataWithOptionalCause>( | |||
There was a problem hiding this comment.
Since we're changing some RPC methods, should we just change it for all the methods?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Going back, if we treat breaking types as breaking then we should make them all optional for now - then there will be the option of making things stricter in the next or an upcoming major.
I could think that perhaps ServerErrorOptions could be stricter then?
There was a problem hiding this comment.
This PR and MetaMask/json-rpc-engine#161 have been updated accordingly.
This is in order to provide backwards-compatibility with code implemented towards v5.0.0.
legobeat
force-pushed
the
optional-error-data
branch
from
5a66807
to
df1102a
Compare
|
Rebased after #103; MetaMask/json-rpc-engine#161 synced. |
|
Digging at MetaMask/providers#253 (similar upgrade error in other package), it seems like there's some drifting in |
| * - A valid DataWithOptionalCause value. | ||
| * - undefined. | ||
| */ | ||
| export type OptionalDataWithOptionalCause = undefined | DataWithOptionalCause; |
There was a problem hiding this comment.
Won't these changes cause issues elsewhere because they aren't valid JSON? Goes for both changes in this file
There was a problem hiding this comment.
The first one should be as fine as it gets, I think.
The one, here, though, I am less confident due to undefined being listed here.
Would it be possible to construct a failure case/regression/validating use-case of some sort? Also, how hard should type-only breakages actually be considered (as they are optional and don't actually affect runtime outcome)?
I'm really struggling to unbreak downstreams in any other way which doesn't cause new detectable inconsistencies and type errors but open to suggestions.
There was a problem hiding this comment.
Actually this should be fine I think. We have serializeError for turning the classes into serializable JSON errors, so having undefined in the class is fine.
This is in order to provide backwards-compatibility with code implemented towards v5.0.0. This fixes type errors introduced for TypeScript users in
5.1.0.Resolves #100