refactor(permission-controller): Clean up use of `any` #4171

rekmarks · 2024-04-16T18:39:51Z

Explanation

Replaces almost all any type casts with @ts-expect-error directives or more specific types. Of the original 81 any type casts in the permission controller and its tests, only 3 remain. No behavioral changes.

Changelog

None.

Checklist

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

Either replaces `any` with `@ts-expect-error` directives or leaves them in place without a TODO to replace them. All remaining instances of `any` are defensible.

… ts-expect-error

hmalik88

Can you explain why things can't be typed where you kept the any and what you mean where you're mentioning abusing types for testing purposes?

hmalik88 · 2024-04-19T01:53:21Z

packages/accounts-controller/src/AccountsController.ts

@@ -302,7 +302,6 @@ export class AccountsController extends BaseController<
        metadata: { ...account.metadata, name: accountName },
      };
      currentState.internalAccounts.accounts[accountId] =
-        // @ts-expect-error Assigning a complex type `T` to `Draft<T>` causes an excessive type instantiation depth error.


So why was this breaking things if it wasn't before?

I have absolutely no idea and I'm scared to find out. Although, it's technically not breaking something now, where it was broken before. Maybe some kind of interaction with the PermissionController types mediated by Snaps-related imports? That's the only thing I can think of.

This upstream bug currently affects Draft<T> types and update() calls in our codebase.

There's not much we can do about it on our end other than add (or remove) @ts-expect-error annotations whenever the compiler breaks out with an "excessive type instantiation depth" error.

For update() calls, returning a new state instead of mutating in place can sometimes help.

What are your thoughts on keeping this comment in place and removing the type assertion instead?

Whatever TS version and config my VS Code is using is unhappy about this, but removing both the cast and the ts-expect-error seems to work: c54671b

packages/permission-controller/src/Caveat.test.ts

packages/permission-controller/src/Caveat.ts

packages/permission-controller/src/PermissionController.ts

packages/permission-controller/src/SubjectMetadataController.ts

packages/permission-controller/src/rpc-methods/getPermissions.test.ts

packages/permission-controller/src/PermissionController.ts

…ByCaveat

rekmarks · 2024-04-19T17:04:12Z

@hmalik88 reposting for visibility:

This is just replacing the previous any types with a @ts-expect-error directive. Either way, we are "abusing" our types by intentionally violating them. The reason for this "abuse" is that we are testing runtime errors that can only be produced through invalid TypeScript.

I appreciate @MajorLift's idiom for this practice:

intentional destructive testing

(Although he rightly accuses these tests of being sloppily typed as opposed to necessarily using any, much of the time.)

….test.ts

packages/permission-controller/src/Caveat.ts

mcmire

One question on keeping the ts-expect-error, but this otherwise looks good to me.

packages/permission-controller/src/Permission.ts

packages/permission-controller/src/PermissionController.test.ts

packages/permission-controller/src/PermissionController.ts

packages/permission-controller/src/PermissionController.test.ts

MajorLift · 2024-04-23T16:04:49Z

I did a final pass and left a few more comments, but aside from those everything lgtm!

MajorLift

Looks good to me! Thanks for your patience with applying all of the suggestions. :)

mcmire

Some nits, but looks good regardless.

packages/permission-controller/src/rpc-methods/requestPermissions.test.ts

packages/permission-controller/src/rpc-methods/revokePermissions.test.ts

mcmire

Looks good!

rekmarks requested a review from a team as a code owner April 16, 2024 18:39

rekmarks force-pushed the rekmarks-permission-controller-cleanup branch from 391be86 to 69c78bb Compare April 16, 2024 18:40

rekmarks changed the title ~~Clean up use of any in PermissionController~~ refactor(permission-controller): Clean up use of any Apr 16, 2024

rekmarks force-pushed the rekmarks-permission-controller-cleanup branch 2 times, most recently from 98cf165 to 87d814d Compare April 17, 2024 15:24

rekmarks mentioned this pull request Apr 17, 2024

feat(permission-controller): Re-throw all well-formed errors #4172

Merged

3 tasks

refactor(permission-controller): Refactor "any" typecasts

0196028

Either replaces `any` with `@ts-expect-error` directives or leaves them in place without a TODO to replace them. All remaining instances of `any` are defensible.

rekmarks force-pushed the rekmarks-permission-controller-cleanup branch from 87d814d to 0196028 Compare April 17, 2024 23:50

fix(accounts-controller): Remove suddenly and mysteriously extraneous…

632ebc9

… ts-expect-error

rekmarks requested a review from a team as a code owner April 17, 2024 23:56

legobeat requested a review from MajorLift April 18, 2024 02:26

hmalik88 reviewed Apr 19, 2024

View reviewed changes

MajorLift reviewed Apr 19, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.ts Outdated Show resolved Hide resolved

refactor(permission-controller): Simplify switch in updatePermissions…

6b95853

…ByCaveat

rekmarks force-pushed the rekmarks-permission-controller-cleanup branch from fdf32e1 to 6b95853 Compare April 19, 2024 16:55

refactor(permission-controller): Replace any types in Caveat.test.ts

f4cd213

rekmarks force-pushed the rekmarks-permission-controller-cleanup branch from c2c596f to f4cd213 Compare April 19, 2024 17:01

refactor(permission-controller): Add specific types in getPermissions…

a16abf9

….test.ts

MajorLift reviewed Apr 19, 2024

View reviewed changes

packages/permission-controller/src/Caveat.ts Show resolved Hide resolved

mcmire reviewed Apr 19, 2024

View reviewed changes

MajorLift reviewed Apr 19, 2024

View reviewed changes

packages/permission-controller/src/Permission.ts Outdated Show resolved Hide resolved

MajorLift reviewed Apr 19, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.test.ts Outdated Show resolved Hide resolved

rekmarks and others added 6 commits April 22, 2024 13:07

Merge branch 'main' into rekmarks-permission-controller-cleanup

e620020

refactor(acounts-controller): Remove ts-expect-error entirely

c54671b

fix: Apply suggestions to Caveat.ts

ab7031b

fix: Apply suggestions to Permission.ts

26e48f1

fix: PermissionController.removeCaveat() type

49d833c

refactor: Replace ts-expect-error directives in tests where possible

376d8f8

rekmarks requested a review from MajorLift April 22, 2024 21:25

rekmarks requested a review from mcmire April 22, 2024 21:25

MajorLift reviewed Apr 23, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.ts Outdated Show resolved Hide resolved

MajorLift reviewed Apr 23, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.test.ts Outdated Show resolved Hide resolved

MajorLift reviewed Apr 23, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.test.ts Outdated Show resolved Hide resolved

MajorLift reviewed Apr 23, 2024

View reviewed changes

packages/permission-controller/src/PermissionController.test.ts Outdated Show resolved Hide resolved

rekmarks and others added 3 commits April 23, 2024 19:09

fix: Apply suggestions to permission controller and its tests

6f8fa3d

fix: Replace all uses of any in permission controller tests

b7a275f

Merge branch 'main' into rekmarks-permission-controller-cleanup

a6b7101

rekmarks requested a review from MajorLift April 24, 2024 02:24

fix: Remove unnecessary any from constraint type

ee2ad01

MajorLift previously approved these changes Apr 24, 2024

View reviewed changes

mcmire previously approved these changes Apr 25, 2024

View reviewed changes

refactor: Replace type casts with type assertions

834d764

rekmarks dismissed stale reviews from mcmire and MajorLift via 834d764 April 25, 2024 23:07

Merge branch 'main' into rekmarks-permission-controller-cleanup

1a948c5

mcmire approved these changes Apr 25, 2024

View reviewed changes

kumavis approved these changes Apr 26, 2024

View reviewed changes

Mrtenz approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'main' into rekmarks-permission-controller-cleanup

1a53dab

rekmarks merged commit 843a794 into main Apr 26, 2024
139 checks passed

rekmarks deleted the rekmarks-permission-controller-cleanup branch April 26, 2024 16:43

rekmarks mentioned this pull request May 1, 2024

Remove any from PermissionController types #4237

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(permission-controller): Clean up use of `any` #4171

refactor(permission-controller): Clean up use of `any` #4171

rekmarks commented Apr 16, 2024 •

edited

Loading

hmalik88 left a comment

hmalik88 Apr 19, 2024

rekmarks Apr 19, 2024

MajorLift Apr 19, 2024

mcmire Apr 19, 2024

rekmarks Apr 22, 2024

rekmarks commented Apr 19, 2024 •

edited

Loading

mcmire left a comment

MajorLift commented Apr 23, 2024

MajorLift left a comment

mcmire left a comment

mcmire left a comment

refactor(permission-controller): Clean up use of any #4171

refactor(permission-controller): Clean up use of any #4171

Conversation

rekmarks commented Apr 16, 2024 • edited Loading

Explanation

Changelog

Checklist

hmalik88 left a comment

Choose a reason for hiding this comment

hmalik88 Apr 19, 2024

Choose a reason for hiding this comment

rekmarks Apr 19, 2024

Choose a reason for hiding this comment

MajorLift Apr 19, 2024

Choose a reason for hiding this comment

mcmire Apr 19, 2024

Choose a reason for hiding this comment

rekmarks Apr 22, 2024

Choose a reason for hiding this comment

rekmarks commented Apr 19, 2024 • edited Loading

mcmire left a comment

Choose a reason for hiding this comment

MajorLift commented Apr 23, 2024

MajorLift left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

refactor(permission-controller): Clean up use of `any` #4171

refactor(permission-controller): Clean up use of `any` #4171

rekmarks commented Apr 16, 2024 •

edited

Loading

rekmarks commented Apr 19, 2024 •

edited

Loading