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

Clean up & standardise TypeScript conventions #2234

Merged
merged 14 commits into from
Mar 31, 2023

Conversation

jovyntls
Copy link
Contributor

@jovyntls jovyntls commented Mar 25, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
As we've been doing quite a large-scale file-by-file migration to TypeScript (#1913), there is some inconsistency and redundancy in how we handle some types, exports, etc. With #2201 and #2221, we now have a more organised typing system and a rough "convention" that can be followed by TS files.

This PR:

  • standardises class exports
  • standardises how we represent generic key-value types to use Record
    • Record<K,V> is interchangeable with { [key: K]: V } syntax, but is more extensible, generalisable, and is an officially supported utility type in TS.
  • fixes some typos in our docs, as many js files are now ts files
  • removes unnecessary as unknown as and any typecasts, now that we have a more mature typing system

Anything you'd like to highlight/discuss:
N/A

Testing instructions:
N/A

Proposed commit message: (wrap lines at 72 characters)

Clean up & standardise TypeScript conventions

The core package is almost fully migrated to TypeScript. There are
several inconsistencies and redundancies in how we represent common
types, handle typecasts, and export/import files. These inconsistencies
may cause confusion, reduce code quality, and prevent us from fully
taking advantage of static typing. 

As of #2201 and #2221, we have a more mature typing system that we can
utilise to fix these inconsistencies as well as a general set of
conventions that can be followed across TS files. 

Let's,
- Standardise class exports
- Standardise key-value types to use `Record<K, V>`
- Remove unnecessary `as unknown as` typecasts
- Reduce the use of `any` types
- Fix outdated docs that refer to `.js` files

Using Record is preferable as `Record<K,V>` is interchangeable with
`{ [key: K]: V }` syntax, but is more extensible and arguably more
readable. `unknown` and `any` types also introduce uncertainty and
unsafe typecasts into the codebase.

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jovyntls jovyntls changed the title Standardise TypeScript Clean up & standardise TypeScript conventions Mar 25, 2023
@jovyntls jovyntls marked this pull request as ready for review March 25, 2023 08:35
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

LGTM

@jovyntls jovyntls requested a review from raysonkoh March 28, 2023 14:48
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

I briefly looked through and looks good to me. Curiously most of the changes in the form of{ [key: string]: string }; to Record<string, string>; will make the type work better or? What's the advantage of Record

@jovyntls
Copy link
Contributor Author

  • Record<K,V> is interchangeable with { [key: K]: V } syntax, but is more extensible, generalisable, and is an officially supported utility type in TS.

@tlylt

  • both Record<K,V> and { [key: K]: V } are currently used in our codebase
  • Record<K,V> is interchangeable with { [key: K]: V } syntax
  • Record<K,V> is more extensible, and is an officially supported utility type in TS
  • Record<K,V> is more flexible, and can support more types for the key (K). It supports generic types which is not supported in { [key: K]: V }.
    • e.g. if we have type ColorName = "red" | "blue"; we can do Record<ColorName, ColorVal> but not { [key: ColorName]: ColorVal }

@jovyntls jovyntls requested a review from tlylt March 30, 2023 14:17
@tlylt tlylt merged commit a43be59 into MarkBind:master Mar 31, 2023
@tlylt tlylt added this to the v4.1.1 milestone Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants