Fix contradictory typing of IFrameMessageResponse and refactor into modular types
#207
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MajorLift
force-pushed
the
231026-typing-fix-iframemessageresponse
branch
from
7e8760d to
d868b03
Compare
MajorLift
changed the title
IFrameMessageResponse non-genericIFrameMessageResponse and refactor for modularity, maintainability
MajorLift
changed the title
IFrameMessageResponse and refactor for modularity, maintainabilityIFrameMessageResponse and refactor for modularity, maintainability
MajorLift
changed the title
IFrameMessageResponse and refactor for modularity, maintainabilityIFrameMessageResponse and refactor into modular types
- `TAction` generic parameter is constrained as a subtype of `IFrameMessageAction` and `IFrameMessageResponse` is only being narrowed, not widened.
…erent/missing properties
MajorLift
force-pushed
the
231026-typing-fix-iframemessageresponse
branch
from
65ba771 to
c2a051f
Compare
mcmire
reviewed
Nov 2, 2023
Co-authored-by: Elliot Winkler <[email protected]>
…avoid confusion regarding atypically-shaped `IFrameMessageResponse` types Co-authored-by: Elliot Winkler <[email protected]>
MajorLift
force-pushed
the
231026-typing-fix-iframemessageresponse
branch
from
01f704a to
9307a30
Compare
legobeat
added a commit
to legobeat/eth-ledger-bridge-keyring
that referenced
this pull request
Dec 2, 2023
…tor into modular types (MetaMask#207)" This reverts commit 2f0da8f.
Merged
legobeat
pushed a commit
to legobeat/eth-ledger-bridge-keyring
that referenced
this pull request
Dec 3, 2023
… modular types (MetaMask#207) ## Motivation `IFrameMessageResponse` is currently defined with a generic parameter `TAction`. This parameter doesn't usefully narrow or widen the type. - The `action: TAction` property appears to *widen* `IFrameMessageResponse` to accept any action type, but `TAction` is also constrained as a subtype of `IFrameMessageActions`. - The `TAction` generic param appears to *narrow* `IFrameMessageResponse` to select individual union members, but it interferes with the inference-based type narrowing we can get if `IFrameMessageResponse` is an exhaustively enumerated discriminated union. This causes two problems: 1. Type errors on assignment operations that should be valid. - e.g. [`as` type casting](https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/207/files#diff-c901ebd8641651c9156a86894574a6d9567f2ae79495de0edff2cbef8ae958f0L302-L304) is necessary here because supertype is being assigned to a subtype i.e. `(response: IFrameMessageResponse<TAction>) => void` to `(response: IFrameMessageResponse<IFrameMessageActions>) => void`. - In general, `(x: SuperType) => T` is a *subtype* of `(x: SubType) => T`. - **This error indicates an underlying issue with the typing and shouldn't be suppressed.** - Removing `TAction` puts `(response: SomeIFrameMessageResponse) => void` on the LHS (assignee, supertype) and `(response: IFrameMessageResponse) => void` on the RHS (assigned, subtype), resolving this logical error. 2. `TAction` is also silencing type errors we should be getting from type narrowing based on `action` value. - e.g. Some union members of `IFrameMessageResponse` do not include a `success`, `error`, `payload`, or `payload.error` property, but because of `TAction`, TypeScript doesn't alert us that we should be performing `in` checks on them in addition to null checks. - This can cause unexpected failure at runtime, especially from the destructuring assignments that are being used. Constraining `IFrameMessageResponse['action']` to `IFrameMessageActions` both resolves these issues and guides us towards writing type-safe logic about these actions that conform to their specific type signatures. It appears that this was the original intention of writing `IFrameMessageActions` as a discriminated union instead of a wider type encompassing all possible actions. ## Changes - **BREAKING** Narrows `IFrameMessageResponse` type definition ## Explanation - To resolve these issues, This PR makes `IFrameMessageResponse` non-generic and removes `as` casting. - For improved readability and maintainability, `IFrameMessageResponse` is also refactored into a union of named types. - A `IFrameMessageResponseBase` type is defined for modularity and better visibility of `IFrameMessageResponse` types with atypical shapes. ## References - Closes MetaMask#208 - More info on function subtyping: MetaMask/core#1890 (comment)
Merged
2 tasks
legobeat
pushed a commit
to legobeat/eth-ledger-bridge-keyring
that referenced
this pull request
Dec 4, 2023
… modular types (MetaMask#207) ## Motivation `IFrameMessageResponse` is currently defined with a generic parameter `TAction`. This parameter doesn't usefully narrow or widen the type. - The `action: TAction` property appears to *widen* `IFrameMessageResponse` to accept any action type, but `TAction` is also constrained as a subtype of `IFrameMessageActions`. - The `TAction` generic param appears to *narrow* `IFrameMessageResponse` to select individual union members, but it interferes with the inference-based type narrowing we can get if `IFrameMessageResponse` is an exhaustively enumerated discriminated union. This causes two problems: 1. Type errors on assignment operations that should be valid. - e.g. [`as` type casting](https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/207/files#diff-c901ebd8641651c9156a86894574a6d9567f2ae79495de0edff2cbef8ae958f0L302-L304) is necessary here because supertype is being assigned to a subtype i.e. `(response: IFrameMessageResponse<TAction>) => void` to `(response: IFrameMessageResponse<IFrameMessageActions>) => void`. - In general, `(x: SuperType) => T` is a *subtype* of `(x: SubType) => T`. - **This error indicates an underlying issue with the typing and shouldn't be suppressed.** - Removing `TAction` puts `(response: SomeIFrameMessageResponse) => void` on the LHS (assignee, supertype) and `(response: IFrameMessageResponse) => void` on the RHS (assigned, subtype), resolving this logical error. 2. `TAction` is also silencing type errors we should be getting from type narrowing based on `action` value. - e.g. Some union members of `IFrameMessageResponse` do not include a `success`, `error`, `payload`, or `payload.error` property, but because of `TAction`, TypeScript doesn't alert us that we should be performing `in` checks on them in addition to null checks. - This can cause unexpected failure at runtime, especially from the destructuring assignments that are being used. Constraining `IFrameMessageResponse['action']` to `IFrameMessageActions` both resolves these issues and guides us towards writing type-safe logic about these actions that conform to their specific type signatures. It appears that this was the original intention of writing `IFrameMessageActions` as a discriminated union instead of a wider type encompassing all possible actions. ## Changes - **BREAKING** Narrows `IFrameMessageResponse` type definition ## Explanation - To resolve these issues, This PR makes `IFrameMessageResponse` non-generic and removes `as` casting. - For improved readability and maintainability, `IFrameMessageResponse` is also refactored into a union of named types. - A `IFrameMessageResponseBase` type is defined for modularity and better visibility of `IFrameMessageResponse` types with atypical shapes. ## References - Closes MetaMask#208 - More info on function subtyping: MetaMask/core#1890 (comment)
legobeat
added a commit
that referenced
this pull request
Dec 4, 2023
… modular types (#207) (#215) Co-authored-by: Jongsun Suh <[email protected]>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
IFrameMessageResponseis currently defined with a generic parameterTAction. This parameter doesn't usefully narrow or widen the type.action: TActionproperty appears to widenIFrameMessageResponseto accept any action type, butTActionis also constrained as a subtype ofIFrameMessageActions.TActiongeneric param appears to narrowIFrameMessageResponseto select individual union members, but it interferes with the inference-based type narrowing we can get ifIFrameMessageResponseis an exhaustively enumerated discriminated union.This causes two problems:
astype casting is necessary here because supertype is being assigned to a subtype i.e.(response: IFrameMessageResponse<TAction>) => voidto(response: IFrameMessageResponse<IFrameMessageActions>) => void.(x: SuperType) => Tis a subtype of(x: SubType) => T.TActionputs(response: SomeIFrameMessageResponse) => voidon the LHS (assignee, supertype) and(response: IFrameMessageResponse) => voidon the RHS (assigned, subtype), resolving this logical error.TActionis also silencing type errors we should be getting from type narrowing based onactionvalue.IFrameMessageResponsedo not include asuccess,error,payload, orpayload.errorproperty, but because ofTAction, TypeScript doesn't alert us that we should be performinginchecks on them in addition to null checks.Constraining
IFrameMessageResponse['action']toIFrameMessageActionsboth resolves these issues and guides us towards writing type-safe logic about these actions that conform to their specific type signatures. It appears that this was the original intention of writingIFrameMessageActionsas a discriminated union instead of a wider type encompassing all possible actions.Changes
IFrameMessageResponsetype definitionExplanation
IFrameMessageResponsenon-generic and removesascasting.IFrameMessageResponseis also refactored into a union of named types.IFrameMessageResponseBasetype is defined for modularity and better visibility ofIFrameMessageResponsetypes with atypical shapes.References
IFrameMessageResponseand refactor into modular types #208base-controller] Fix allanyusage, apply universal supertype for functions core#1890 (comment)