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

Add entry forany, as usage in TypeScript Guidelines doc #47

Closed
MajorLift opened this issue Sep 27, 2023 · 0 comments · Fixed by #74
Closed

Add entry forany, as usage in TypeScript Guidelines doc #47

MajorLift opened this issue Sep 27, 2023 · 0 comments · Fixed by #74
Assignees

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Sep 27, 2023

Motivation

  • There are currently hundreds of instances of any type usage and as assertions spread throughout metamask-extension and core.
  • For future maintainability, there should be clear guidelines defining the limited cases where any, as usage is intentional and acceptable vs. the majority of cases where any, as should be corrected and removed.

References

Research

Catalogue any, as usage in core, metamask-extension and apply fixes where appropriate.

any

Avoiding any

  • any doesn't provide the widest type, or any type at all. It's a compiler setting to disable type checking for the type it's assigned to, which kneecaps type safety by definition.

  • unknown is the universal supertype i.e. the widest possible type.

    • If what's needed is a type that could be "anything," unknown should be used.
    • any and unknown are interchangeable when typing the assignee (every type is assignable to both).
    • any is assignable to all types, while unknown is only assignable to unknown.
    • If replacing any with unknown doesn't work, the typing likely has underlying issues that shouldn't be silenced.
  • If any is used, and errors are introduced (or altered) by future changes to the code,

    • the new or changed warnings will be suppressed,
    • and the code will fail silently.
    • "dangerous because infects all surrounding and downstream code ..."
  • If no good typing solution can be found, using forced type casting with the as keyword or even as unknown as is much preferable to using any:

    • At least we get working intellisense, autocomplete.
    • We also get an indication of the expected type as intended by the author.
  • Function supertype (assignable to any function):

(...args: any[]) => any // bad
(...args: never[]) => unknown // good

Acceptable usages of any

  • Assigning new properties to an object or class at runtime:
(this as any)[key] = obj[key]
  • Within generic constraints:
messenger extends RestrictedControllerMessenger<N, any, any, string, string>
    • In general, using any in this context is not harmful in the same way that it is in other contexts, as the any types are not directly assigned to any specific variable, and only function as constraints.
    • That said, narrower constraints provide better type safety and intellisense.
  • Catching errors:
    • catch only accepts any and unknown as the error type .
    • Recommended: Use unknown with type guards like isJsonRpcError .
  • In tests, for mocking or to intentionally break features.
    • Recommended: Provide accurate typing through assertions wherever possible.

as

Avoiding as

Consider replacing as casting with the following, in order of preference:

  • Prefer type inference.
    • Inferences are responsive to changes in code, while assertions rely on brittle hard-coding. While TypeScript will throw type errors against some unsafe or structurally unsound type assertion scenarios, it will generally accept the user-supplied type without type-checking. This can cause silent failures where errors are suppressed.
    • In some cases, type assertions can be replaced with type narrowing with type guards and null checks.
  • If a type constraint needs to be imposed, prefer the satisfies keyword (for TypeScript v4.9+), since it allows type inference to a more narrow type.
  • : type declarations also enable type constraints to be imposed, but don't allow narrowing by inference.
  • as assertions intended to exclude an empty/nullable type can be replaced by a nullish coalescing operator converting nullable values into an acceptable empty value that doesn't pollute the variable's type signature.
(arr as string[])
(arr ?? [])

Acceptable usages of as

  • To define user-defined type guards: https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
  • To type inputs/outputs that are defined at runtime, provided that schema validation is performed with type guards and unit tests.
    • e.g. The output of JSON.parse() or await response.json() for a known JSON input.
  • In tests, for mocking objects.
    • as unknown as can be used where the mocked object and expected type are significantly divergent e.g. due to omitting required properties.
{ ...mockObject } as unknown as TargetType
@MajorLift MajorLift self-assigned this Sep 27, 2023
@MajorLift MajorLift changed the title Add entry forany usage in TypeScript Guidelines doc Add entry forany, as usage in TypeScript Guidelines doc Oct 11, 2023
MajorLift added a commit to MetaMask/auto-changelog that referenced this issue Oct 13, 2023
- `as` assertions intended to exclude an empty/nullable type can be replaced by a nullish coalescing operator converting nullable values into an acceptable empty value that doesn't pollute the variable's type signature.
- TODO: document and expound in MetaMask/contributor-docs#47
MajorLift added a commit to MetaMask/auto-changelog that referenced this issue Oct 13, 2023
Sometimes TypeScript's type narrowing and inference doesn't fully work quite like it should. Even in these cases, using `as` assertions should be avoided.

It may seem like we know for certain what the type should be and there's no risk of it breaking, but there's always the possibility that some combination of changes elsewhere might affect the typing, in which case `as` will suppress the alerts TypeScript would otherwise provide us with. This may lead to code breaking silently.

In this case, adding type guards and null checks -- even if they're redundant -- is preferable to the above scenario.

- TODO: document and expound in MetaMask/contributor-docs#47
MajorLift added a commit to MetaMask/auto-changelog that referenced this issue Oct 13, 2023
If anything changes in the typing of `ChangeCategory` or `unreleasedChanges` that make them inconsistent, this code will fail silently. Relying on type inference instead of explicit type casting will make these line a little less brittle.

- TODO: document and expound in MetaMask/contributor-docs#47
MajorLift added a commit to MetaMask/auto-changelog that referenced this issue Oct 17, 2023
…#158)

* Replace `as T[]` assertions with `?? []`

- `as` assertions intended to exclude an empty/nullable type can be replaced by a nullish coalescing operator converting nullable values into an acceptable empty value that doesn't pollute the variable's type signature.
- TODO: document and expound in MetaMask/contributor-docs#47

* Replace `as` assertions with redundant type guards

Sometimes TypeScript's type narrowing and inference doesn't fully work quite like it should. Even in these cases, using `as` assertions should be avoided.

It may seem like we know for certain what the type should be and there's no risk of it breaking, but there's always the possibility that some combination of changes elsewhere might affect the typing, in which case `as` will suppress the alerts TypeScript would otherwise provide us with. This may lead to code breaking silently.

In this case, adding type guards and null checks -- even if they're redundant -- is preferable to the above scenario.

- TODO: document and expound in MetaMask/contributor-docs#47

* Make `as` assertion rely on inference instead of hard-coded type casting

If anything changes in the typing of `ChangeCategory` or `unreleasedChanges` that make them inconsistent, this code will fail silently. Relying on type inference instead of explicit type casting will make these line a little less brittle.

- TODO: document and expound in MetaMask/contributor-docs#47

* Fix type narrowing for `currentVersion` by consolidating conditional branches

- more consistent logic for `isReleaseCandidate` null check
- wasted processing overhead due to errors throwing later than strictly necessary

* Install `@metamask/utils` as a devDep

* Replace `Object.keys` with `getKnownPropertyNames` method

- removes need for type casting while iterating over object keys

* Update src/update-changelog.ts
MajorLift added a commit that referenced this issue Apr 30, 2024
…cape Hatches (#74)

## Motivation

In order to improve type safety and maintainability, we need to
establish clear guidelines regarding how to apply types: specifically,
when and when not to use explicit type declarations or keywords such as
`as`, `any`.

## Explanation

- See markdown preview: [TypeScript Guidelines - "Types"
section](https://github.com/MetaMask/contributor-docs/blob/240206-typescript-guidelines-any-as/docs/typescript.md)
- ~The merge conflicts with main will be resolved after the review
process to avoid noisy diffs.~
- All examples are tagged with a permalink for easy reference and
quotation.

### Table of Contents
- Types
  - Type Inference
  - Type Annotations (`:`, `satisfies`)
  - Type Assertions (`as`, `!`)
  - Escape Hatches (`any`, `@ts-expect-error`)
    
### Details
- Dangers and advantages.
- Tips for avoiding.
- Acceptable use cases.

## References
- Closes #47
- Closes #57 
- Contributes to #69

---------

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
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
1 participant