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

ADR for Typescript Guidance #412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marlonkeating
Copy link
Contributor

This ADR elaborates on the rationale for using TypeScript, areas where migration should be prioritized (and not), and guidance on doing minimalist TypeScript introductions into new code bases.

### Target Goals
Prioritize using TypeScript in the following places:
* New code files
* Existing API endpoints (and their payloads)
Copy link
Member

Choose a reason for hiding this comment

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

[curious] If we add types for expected responses from API endpoints, I might be curious how those would interact, if at all, with Pact contract testing if/when we adopt contract testing more broadly throughout the platform. For example, in a world with contract testing via Pact, would we need to implement TypeScript types for API responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I have seen of contract testing, you wouldn't need to provide TypeScript types, but they would surely help in writing contract tests. Overall, I would consider contract testing and TypeScript types to be supplementary means of ensuring that the api contracts are adhered to.

Prioritize using TypeScript in the following places:
* New code files
* Existing API endpoints (and their payloads)
* Components or Functions take a lot of parameters, or use parameters that are themselves complex objects
Copy link
Member

Choose a reason for hiding this comment

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

[suggestion] I might also include types around things like React context values, as well.

* React components without complex input objects (especially since they are often adequately served with PropTypes)
* Test code

### Migration Strategy
Copy link
Member

Choose a reason for hiding this comment

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

[aside] We may want to consider when it makes sense to get our TypeScript supported alpha release of @edx/frontend-build merged into master to be published on the main distribution channel for NPM. At what point will we feel confident that merging alpha into master won't cause any regressions for non-TypeScript repos, in their build/test/lint/release pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my mind, the safe time to do it is when all of frontend-build's consuming repos either
A) Are consuming frontend-build's alpha release with TypeScript in place
B) Have at least a working draft PR of consuming the alpha release with all the pipeline checks passing.

Does that sound stringent enough (and practical given the number of consumers)?


### Challenges
* Migrating to TypeScript falls into the category of Tech Debt, which is difficult to get around to around the demands of mission-critical feature work and bug fixes.
* There can be a bit of a learning curve around using TypeScript, particularly for Javascript developers who don't have prior experience with strongly-typed languages.
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth adding an "Alternatives considered" section with some points such as:

  • Flow. It's another popular type system for JS; has largely been outpopularized by TypeScript.
  • Continuing to use and formalize JSDoc docstrings. For example, these can give you some of the same type hinting benefits in IDEs as TypeScript, but is not as feature rich.
  • Do nothing and stick with regular ol' JavaScript.

Here are some of the advantages of TypeScript over JavaScript:

### Type safety
TypeScript helps catch errors at compile-time instead of runtime by adding type annotations to variables, functions, and classes. This can help prevent errors that might occur when dealing with large codebases.
Copy link
Member

Choose a reason for hiding this comment

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

[curious] I wonder if it's worth including any benefits of type safety regarding refactoring, specifically?

### Type safety
TypeScript helps catch errors at compile-time instead of runtime by adding type annotations to variables, functions, and classes. This can help prevent errors that might occur when dealing with large codebases.
### Better tooling support
TypeScript has better tooling support than JavaScript, with features like code navigation, auto-completion, and refactoring tools in popular code editors like Visual Studio Code.
Copy link
Member

Choose a reason for hiding this comment

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

[suggestion] Including a mention of generating API documentation from TypeScript might be worthwhile? i believe there's improved tool for auto-generated TypeScript docs compared to JSDocs documentation generation. (That said, I don't have much personal experience here.)

Here are some of the advantages of TypeScript over JavaScript:

### Type safety
TypeScript helps catch errors at compile-time instead of runtime by adding type annotations to variables, functions, and classes. This can help prevent errors that might occur when dealing with large codebases.
Copy link
Member

Choose a reason for hiding this comment

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

Another potential couple resources to include that objectively quantifies some of the advantages/disadvantages of TypeScript:

  • https://earlbarr.com/publications/typestudy.pdf
    • Roughly 15-20% of bugs catchable by TypeScript. A bit dated now, from back in 2017.
  • To_Type_or_Not_to_Type_A_Systematic_Comparison_of_.pdf
    • More recent, from 2022. These results suggest that while TypeScript does have some benefits on code quality/understandability, it may be at the expense of other things such as slightly longer MTTR for bugs, which is IMO is particularly interesting and may be worth diving a bit into in the "Consequences" section.
    • "The analysis indicates that TS applications exhibit significantly better code quality and understandability than JS applications. Contrary to expectations, however, bug proneness and bug resolution time of our TS sample were not significantly lower than for JS: the mean bug fix commit ratio of TS projects was more than 60% larger (0.126 vs. 0.206), and TS projects needed on average more than an additional day to fix bugs (31.86 vs. 33.04 days). Furthermore, reducing the usage of the any type in TS apps appears to be beneficial: its frequency was significantly correlated with all metrics except bug proneness, even though the correlations were of small strengths (Spearman’s rho between 0.17 and 0.26)."

Interestingly, it seems some bodies of research (blog post) suggest that introducing a practice of Test-Driven Development (TDD) has significantly more reduction in bug density compared to type safety with TypeScript, given TDD led to 40-80% reductions in bug density. If one of the goals of adopting TypeScript is to reduce bug density across the Open edX platform, some research seems to suggest that TypeScript may not be as significant of a driver as some might think where it's actually more advantageous in other areas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the resources! Given my own past work in statically-typed languages, I know that there are plenty of bug categories that won't be be caught by just adding type-checking, that I expect something like TDD would help with (given it addresses behavior as well as contract enforcement).

Suffice it to say, I'm very much in favor of a TypeScript and TDD approach.

* Components or Functions take a lot of parameters, or use parameters that are themselves complex objects

Less important are:
* React components without complex input objects (especially since they are often adequately served with PropTypes)
Copy link
Member

Choose a reason for hiding this comment

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

[curious] I'm wondering whether having to decide between adding types via PropTypes or TypeScript types being dependent on something relatively subjective (i.e., define "complex") could lead to some potential confusion around when to use which, when, and why. In some situations, we may want/need both. For example, in this Paragon PR to add types to all components, both TypeScript types and PropTypes are used together. Granted, Paragon's Gatsby documentation website relies on the PropTypes object to generate the props API.
image

docs: feedback additions
@marlonkeating marlonkeating force-pushed the mkeating/typescript-adr branch from dab939a to 6060279 Compare July 14, 2023 14:53
@Mashal-m
Copy link
Contributor

Hey @marlonkeating, is it ready to review and merge?

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.

3 participants