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

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

Merged
merged 18 commits into from
Apr 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0196028
refactor(permission-controller): Refactor "any" typecasts
rekmarks Apr 16, 2024
632ebc9
fix(accounts-controller): Remove suddenly and mysteriously extraneous…
rekmarks Apr 17, 2024
6b95853
refactor(permission-controller): Simplify switch in updatePermissions…
rekmarks Apr 19, 2024
f4cd213
refactor(permission-controller): Replace `any` types in Caveat.test.ts
rekmarks Apr 19, 2024
a16abf9
refactor(permission-controller): Add specific types in getPermissions…
rekmarks Apr 19, 2024
e620020
Merge branch 'main' into rekmarks-permission-controller-cleanup
rekmarks Apr 22, 2024
c54671b
refactor(acounts-controller): Remove ts-expect-error entirely
rekmarks Apr 22, 2024
ab7031b
fix: Apply suggestions to Caveat.ts
rekmarks Apr 22, 2024
26e48f1
fix: Apply suggestions to Permission.ts
rekmarks Apr 22, 2024
49d833c
fix: PermissionController.removeCaveat() type
rekmarks Apr 22, 2024
376d8f8
refactor: Replace ts-expect-error directives in tests where possible
rekmarks Apr 22, 2024
6f8fa3d
fix: Apply suggestions to permission controller and its tests
rekmarks Apr 24, 2024
b7a275f
fix: Replace all uses of any in permission controller tests
rekmarks Apr 24, 2024
a6b7101
Merge branch 'main' into rekmarks-permission-controller-cleanup
rekmarks Apr 24, 2024
ee2ad01
fix: Remove unnecessary any from constraint type
rekmarks Apr 24, 2024
834d764
refactor: Replace type casts with type assertions
rekmarks Apr 25, 2024
1a948c5
Merge branch 'main' into rekmarks-permission-controller-cleanup
rekmarks Apr 25, 2024
1a53dab
Merge branch 'main' into rekmarks-permission-controller-cleanup
rekmarks Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/accounts-controller/src/AccountsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,7 @@ export class AccountsController extends BaseController<
...account,
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

internalAccount as Draft<InternalAccount>;
currentState.internalAccounts.accounts[accountId] = internalAccount;
});
}

Expand Down
42 changes: 21 additions & 21 deletions packages/permission-controller/src/Caveat.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { PermissionConstraint } from '.';
import type { Caveat, PermissionConstraint } from '.';
import { decorateWithCaveats, PermissionType } from '.';
import * as errors from './errors';

Expand All @@ -9,11 +9,11 @@ describe('decorateWithCaveats', () => {
const caveatSpecifications = {
reverse: {
type: 'reverse',
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
rekmarks marked this conversation as resolved.
Show resolved Hide resolved
decorator: (method: any, _caveat: any) => async () => {
return (await method()).reverse();
},
decorator:
(method: () => Promise<unknown[]>, _caveat: Caveat<string, null>) =>
async () => {
return (await method()).reverse();
},
},
};

Expand Down Expand Up @@ -46,19 +46,19 @@ describe('decorateWithCaveats', () => {
const caveatSpecifications = {
reverse: {
type: 'reverse',
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
decorator: (method: any, _caveat: any) => async () => {
return (await method()).reverse();
},
decorator:
(method: () => Promise<unknown[]>, _caveat: Caveat<string, null>) =>
async () => {
return (await method()).reverse();
},
},
slice: {
type: 'slice',
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
decorator: (method: any, caveat: any) => async () => {
return (await method()).slice(0, caveat.value);
},
decorator:
(method: () => Promise<unknown[]>, caveat: Caveat<string, number>) =>
async () => {
return (await method()).slice(0, caveat.value);
},
},
};

Expand Down Expand Up @@ -114,11 +114,11 @@ describe('decorateWithCaveats', () => {
const caveatSpecifications = {
reverse: {
type: 'reverse',
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
decorator: (method: any, _caveat: any) => async () => {
return (await method()).reverse();
},
decorator:
(method: () => Promise<unknown[]>, _caveat: Caveat<string, null>) =>
async () => {
return (await method()).reverse();
},
},
};

Expand Down
61 changes: 26 additions & 35 deletions packages/permission-controller/src/Caveat.ts
View file
Open in desktop
rekmarks marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,16 @@ export type CaveatDecorator<ParentCaveat extends CaveatConstraint> = (
* @template Decorator - The {@link CaveatDecorator} to extract a caveat value
* type from.
*/
// TODO: Replace `any` with type
rekmarks marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line @typescript-eslint/no-explicit-any
type ExtractCaveatValueFromDecorator<Decorator extends CaveatDecorator<any>> =
Decorator extends (
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
decorated: any,
caveat: infer ParentCaveat,
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
) => AsyncRestrictedMethod<any, any>
? ParentCaveat extends CaveatConstraint
? ParentCaveat['value']
: never
: never;
type ExtractCaveatValueFromDecorator<
Decorator extends CaveatDecorator<CaveatConstraint>,
> = Decorator extends (
decorated: AsyncRestrictedMethod<RestrictedMethodParameters, Json>,
caveat: infer ParentCaveat,
) => AsyncRestrictedMethod<RestrictedMethodParameters, Json>
? ParentCaveat extends CaveatConstraint
? ParentCaveat['value']
: never
: never;

/**
* A function for validating caveats of a particular type.
Expand Down Expand Up @@ -140,9 +135,7 @@ export type RestrictedMethodCaveatSpecificationConstraint =
* The decorator function used to apply the caveat to restricted method
* requests.
*/
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
decorator: CaveatDecorator<any>;
decorator: CaveatDecorator<CaveatConstraint>;
};

export type EndowmentCaveatSpecificationConstraint = CaveatSpecificationBase;
Expand Down Expand Up @@ -180,9 +173,10 @@ type CaveatSpecificationBuilderOptions<
* tailored to their requirements.
*/
export type CaveatSpecificationBuilder<
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Options extends CaveatSpecificationBuilderOptions<any, any>,
Options extends CaveatSpecificationBuilderOptions<
Record<string, unknown>,
Record<string, unknown>
>,
Specification extends CaveatSpecificationConstraint,
> = (options: Options) => Specification;

Expand All @@ -192,9 +186,10 @@ export type CaveatSpecificationBuilder<
*/
export type CaveatSpecificationBuilderExportConstraint = {
specificationBuilder: CaveatSpecificationBuilder<
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
CaveatSpecificationBuilderOptions<any, any>,
CaveatSpecificationBuilderOptions<
Record<string, unknown>,
Record<string, unknown>
>,
CaveatSpecificationConstraint
>;
decoratorHookNames?: Record<string, true>;
Expand All @@ -220,18 +215,14 @@ export type CaveatSpecificationMap<
*/
export type ExtractCaveats<
CaveatSpecification extends CaveatSpecificationConstraint,
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
> = CaveatSpecification extends any
? CaveatSpecification extends RestrictedMethodCaveatSpecificationConstraint
? Caveat<
CaveatSpecification['type'],
ExtractCaveatValueFromDecorator<
RestrictedMethodCaveatSpecificationConstraint['decorator']
>
> = CaveatSpecification extends RestrictedMethodCaveatSpecificationConstraint
? Caveat<
CaveatSpecification['type'],
ExtractCaveatValueFromDecorator<
RestrictedMethodCaveatSpecificationConstraint['decorator']
>
: Caveat<CaveatSpecification['type'], Json>
: never;
>
: Caveat<CaveatSpecification['type'], Json>;

/**
* Extracts the type of a specific {@link Caveat} from a union of caveat
Expand Down
39 changes: 18 additions & 21 deletions packages/permission-controller/src/Permission.ts
View file
Open in desktop
rekmarks marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ export type ValidPermission<
*/
type ExtractArrayMembers<ArrayType> = ArrayType extends []
? never
: // TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
ArrayType extends any[] | readonly any[]
: ArrayType extends unknown[] | readonly unknown[]
? ArrayType[number]
: never;

Expand Down Expand Up @@ -209,9 +207,7 @@ export type RequestedPermissions = Record<TargetName, RequestedPermission>;
*/
type RestrictedMethodContext = Readonly<{
origin: OriginString;
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
[key: string]: unknown;
}>;

export type RestrictedMethodParameters = Json[] | Record<string, Json>;
Expand Down Expand Up @@ -265,9 +261,10 @@ export type RestrictedMethod<
| AsyncRestrictedMethod<Params, Result>;

export type ValidRestrictedMethod<
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
MethodImplementation extends RestrictedMethod<any, any>,
MethodImplementation extends RestrictedMethod<
RestrictedMethodParameters,
Json
>,
> = MethodImplementation extends (args: infer Options) => Json | Promise<Json>
? Options extends RestrictedMethodOptions<RestrictedMethodParameters>
? MethodImplementation
Expand Down Expand Up @@ -423,9 +420,7 @@ type PermissionSpecificationBase<Type extends PermissionType> = {
*
* If the side-effect action fails, the permission that triggered it is revoked.
*/
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sideEffect?: PermissionSideEffect<any, any>;
sideEffect?: PermissionSideEffect<ActionConstraint, EventConstraint>;

/**
* The Permission may be available to only a subset of the subject types. If so, specify the subject types as an array.
Expand Down Expand Up @@ -469,9 +464,7 @@ export type EndowmentSpecificationConstraint =
* permission is invoked, after which the host can apply the endowments to
* the requesting subject in the intended manner.
*/
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
endowmentGetter: EndowmentGetter<any>;
endowmentGetter: EndowmentGetter<Json>;
};

/**
Expand Down Expand Up @@ -511,9 +504,11 @@ type PermissionSpecificationBuilderOptions<
*/
export type PermissionSpecificationBuilder<
Type extends PermissionType,
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Options extends PermissionSpecificationBuilderOptions<any, any, any>,
Options extends PermissionSpecificationBuilderOptions<
Record<string, unknown>,
Record<string, unknown>,
Record<string, unknown>
>,
Specification extends PermissionSpecificationConstraint & {
permissionType: Type;
},
Expand All @@ -527,9 +522,11 @@ export type PermissionSpecificationBuilderExportConstraint = {
targetName: string;
specificationBuilder: PermissionSpecificationBuilder<
PermissionType,
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
PermissionSpecificationBuilderOptions<any, any, any>,
PermissionSpecificationBuilderOptions<
Record<string, unknown>,
Record<string, unknown>,
Record<string, unknown>
>,
PermissionSpecificationConstraint
>;
factoryHookNames?: Record<string, true>;
Expand Down
Loading
Loading