[composable-controller] Better typing for state, messenger, strict type checks for inputs, remove #controllers field
#3904
Conversation
MajorLift
force-pushed
the
230207-fix-composable-controller
branch
from
a00ed44
to
3d8f0f6
Compare
MajorLift
changed the title
#updateChildController behavior and improve typing
MajorLift
force-pushed
the
230207-fix-composable-controller
branch
9 times, most recently
from
6c5b5de
to
818075d
Compare
MajorLift
changed the title
#updateChildController behavior and improve typing#controllers field, better typing for state, strict type checks for inputs
| { | ||
| "path": "../json-rpc-engine" | ||
| } |
There was a problem hiding this comment.
Added json-rpc-engine as a TypeScript project reference (but not in the build tsconfig) instead of introducing it as a dependency, since it's only being used in a test file.
There was a problem hiding this comment.
Do we also want to add json-rpc-engine as a dev dependency? Maybe it makes no difference from a TypeScript perspective, but it communicates the dependency from a Yarn/NPM perspective.
There was a problem hiding this comment.
Fixed here: 5da0893
Will a json-rpc-engine project reference need to be added to tsconfig.build.json as well?
There was a problem hiding this comment.
If it's only being used for tests, then you're correct, I don't think adding it to tsconfig.build.json would be needed. You should be able to test this by running yarn build — you shouldn't see any errors.
There was a problem hiding this comment.
No errors on yarn build! Think we're good to go.
MajorLift
force-pushed
the
230207-fix-composable-controller
branch
3 times, most recently
from
b4fe231
to
3b50a63
Compare
| // `any` is used here to disable the `BaseController` type constraint which expects state properties to extend `Record<string, Json>`. | ||
| // `ComposableController` state needs to accommodate `BaseControllerV1` state objects that may have properties wider than `Json`. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| [name: string]: Record<string, any>; |
There was a problem hiding this comment.
Previous typing of ControllerInstance['state'] implicitly evaluated as any.
This is the error if any is replaced with something wider than Json e.g. unknown:
Type 'ComposableControllerState' does not satisfy the constraint 'Record<string, Json>'.
'string' index signatures are incompatible.
Type 'Record<string, unknown>' is not assignable to type 'Json'.
| }; | ||
|
|
||
| export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< | ||
| typeof controllerName, | ||
| ComposableControllerState | ||
| Record<string, unknown> |
There was a problem hiding this comment.
Ideally, this would be something that covers the state types of both BaseController versions:
Record<string, BaseState & Record<string, unknown> | Record<string, Json>>But this results in an implicit any error, and it conflicts with the messenger typing for allowed events. Using unknown currently seems to be the best option.
To be revisited once AllowedEvents is narrowed by #3627.
| string, | ||
| string | ||
| ComposableControllerEvents | AllowedEvents, | ||
| never, |
There was a problem hiding this comment.
No reason to keep this as string since ComposableController doesn't use any actions from other controllers.
MajorLift
force-pushed
the
230207-fix-composable-controller
branch
from
529eb3b
to
2e551e2
Compare
| throw new Error( | ||
| 'Invalid controller: controller must extend from BaseController or BaseControllerV1', |
There was a problem hiding this comment.
Test case added for this branch.
| (childState: Record<string, unknown>) => { | ||
| } else if (isBaseController(controller)) { | ||
| this.messagingSystem.subscribe(`${name}:stateChange`, (childState) => { | ||
| if (isValidJson(childState)) { |
There was a problem hiding this comment.
Since the child controller is validated as BaseControllerV2, its state type is guaranteed to be Record<string, Json>. It's therefore safe to narrow childState from Record<string, unknown>.
| ): controller is BaseControllerV1< | ||
| BaseConfig & Record<string, unknown>, | ||
| BaseState & Record<string, unknown> | ||
| > { | ||
| return ( | ||
| 'name' in controller && | ||
| typeof controller.name === 'string' && | ||
| 'defaultConfig' in controller && | ||
| typeof controller.defaultConfig === 'object' && | ||
| 'defaultState' in controller && | ||
| typeof controller.defaultState === 'object' && | ||
| 'disabled' in controller && | ||
| typeof controller.disabled === 'boolean' && | ||
| controller instanceof BaseControllerV1 | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Determines if the given controller is an instance of BaseController | ||
| * @param controller - Controller instance to check | ||
| * @returns True if the controller is an instance of BaseController | ||
| */ | ||
| export function isBaseController( | ||
| controller: ControllerInstance, | ||
| ): controller is BaseController<never, never, never> { | ||
| return ( | ||
| 'name' in controller && | ||
| typeof controller.name === 'string' && | ||
| 'state' in controller && | ||
| typeof controller.state === 'object' && | ||
| controller instanceof BaseController | ||
| ); |
There was a problem hiding this comment.
Added redundant property checks for extra runtime safety.
| return controller instanceof BaseControllerV1; | ||
| ): controller is BaseControllerV1< | ||
| BaseConfig & Record<string, unknown>, | ||
| BaseState & Record<string, unknown> |
There was a problem hiding this comment.
The BaseControllerV1 childState in #updateChildController is correctly inferred to BaseState & Record<string, unknown>.
MajorLift
changed the title
#controllers field, better typing for state, strict type checks for inputs#controllers field
| @@ -63,8 +104,6 @@ export class ComposableController extends BaseController< | |||
| ComposableControllerState, | |||
| ComposableControllerMessenger | |||
| > { | |||
| readonly #controllers: ControllerList = []; | |||
There was a problem hiding this comment.
Alternatively, we could keep the #controllers field and add something like the following to #updateChildController.
if (!this.state[name]) {
this.#controllers.push(controller)
}I prefer removing the field because it makes it unambiguous that child controllers aren't meant to be added, and their state and subscriptions aren't meant to be mutated after instantiation.
Also, having ComposableController depend on mutable internal state would introduce complications for #3627.
This reverts commit 280e8e4.
…ontrollers have migrated to V2
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Linter fix
MajorLift
force-pushed
the
230207-fix-composable-controller
branch
from
bd8905a
to
856001b
Compare
Explanation
There are several issues with the current ComposableController implementation that should be addressed before further updates are made to the controller (e.g. #3627).
#controllersclass field, which is not being updated by#updateChildControlleror anywhere else.#updateChildControllerbeing a private method.Record<string, Json>anyto disable BaseController state type constraint, as there is no straightforward way to typeBaseControllerV1state to be compatible withJson.isBaseControllertype guard,and removes the deprecated.subscribedproperty fromBaseControllerControllerListtype in anticipation of [composable-controller] Make class and messenger generic upon child controllers #3627. Internally,ControllerInstancewill be used to type child controllers or unions and tuples thereof.BaseControllerV{1,2}Instancetypes.References
composable-controller: Replace use ofanywith proper types (non-test files only) #3716anyusage.#updateChildControllerbehavior and improve typing #3907Changelog
Recorded under "Unreleased" heading in CHANGELOG files.
Checklist