Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

composable-controller: Replace use of any with proper types (non-test files only) #3716

Closed
Tracked by #3706
mcmire opened this issue Jan 4, 2024 · 0 comments · Fixed by #3904
Closed
Tracked by #3706

composable-controller: Replace use of any with proper types (non-test files only) #3716

mcmire opened this issue Jan 4, 2024 · 0 comments · Fixed by #3904
Assignees

Comments

@mcmire
Copy link
Contributor

mcmire commented Jan 4, 2024

Look for the following string in non-test files to find these: // TODO: Replace `any` with type

At time of writing, there were 2 instances.

@MajorLift MajorLift self-assigned this Feb 21, 2024
MajorLift added a commit that referenced this issue Feb 21, 2024
…pe checks for inputs, remove `#controllers` field (#3904)

## 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).

- Removes `#controllers` class field, which is not being updated by
`#updateChildController` or anywhere else.
- Removing this makes it clear that the list of child controllers to be
composed is determined at class instantiation and cannot be altered
later.
- This behavior is consistent with `#updateChildController` being a
private method.
- Types BaseController, ComposableController state with `Record<string,
Json>`
- Opted to use `any` to disable BaseController state type constraint, as
there is no straightforward way to type `BaseControllerV1` state to be
compatible with `Json`.
- Adds a `isBaseController` type guard, ~and removes the deprecated
`subscribed` property from `BaseController`~.
- Removes `ControllerList` type in anticipation of
#3627. Internally,
`ControllerInstance` will be used to type child controllers or unions
and tuples thereof.
- Adds and exports `ControllerInstance`, `BaseControllerV{1,2}Instance` types.
- Populates state metadata object in constructor with child controllers.

## References

- Blocks #3627 
- Closes #3716
- Replaces some TODOs with comment explaining necessity of `any` usage.
- Closes #3907

## Changelog

Recorded under "Unreleased" heading in CHANGELOG files.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants