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.

Sign up for GitHub

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

Jump to bottom

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

Closed
Tracked by #3706
mcmire opened this issue Jan 4, 2024 · 1 comment · Fixed by #3959
Closed
Tracked by #3706

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

mcmire opened this issue Jan 4, 2024 · 1 comment · Fixed by #3959
Assignees
@MajorLift
Labels
team-wallet-framework

Comments

@mcmire
Copy link
Contributor

mcmire commented Jan 4, 2024 •
edited by desi
Loading

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

These are occurring in BaseControllerV1 - so we are deciding to not address this.
@mcmire mcmire mentioned this issue Jan 4, 2024
Open
19 tasks
@MajorLift
Copy link
Contributor

MajorLift commented Feb 8, 2024 •
edited
Loading

Both instances are examples where as any is used to add new properties to an object at runtime. This is a use case that will be listed as acceptable in the TypeScript guidelines any section.

We could replace the TODOs with comments that explain this fact and point to contributor-docs. This would serve as a good example case for 2024/Q1/O2/KR2.

If we go ahead with this approach, we should add MetaMask/contributor-docs#47 as a blocker for this ticket.

@MajorLift MajorLift self-assigned this Feb 22, 2024
@MajorLift MajorLift mentioned this issue Feb 22, 2024
Merged
3 tasks
MajorLift added a commit that referenced this issue Feb 27, 2024
@MajorLift
[base-controller] Fix any usage in BaseControllerV1 (#3959)
eda5b9d 
## Explanation

- For runtime property assignment, use `as unknown as` instead of `as
any`.
- Change the types for `BaseControllerV1` class fields `initialConfig`,
`initialState` from `C`, `S` to `Partial<C>`, `Partial<S>`.
- Initial user-supplied constructor options do not need to be complete
`C`, `S` objects, since `internal{Config,State}` will be populated with
`default{Config,State}`.
- For empty objects, prefer no type assertions or `as never` (`never` is
assignable to all types).
- Fix code written based on outdated TypeScript limitation.
- Generic spread expressions for object literals are supported by
TypeScript: microsoft/TypeScript#28234

## References

- Closes #3715 

## Changelog

###
[`@metamask/base-controller`](https://github.com/MetaMask/core/pull/3959/files#diff-a8212838da15b445582e5622bd4cc8195e4c52bcf87210af8074555f806706a9)

## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MajorLift MajorLift

Labels
team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[base-controller] Fix any usage in BaseControllerV1 MetaMask/core
2 participants
@mcmire @MajorLift