-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: master
Are you sure you want to change the base?
Conversation
4276947
to
dab939a
Compare
### Target Goals | ||
Prioritize using TypeScript in the following places: | ||
* New code files | ||
* Existing API endpoints (and their payloads) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
docs/0003-typescript-guidance.md
Outdated
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.)
docs/0003-typescript-guidance.md
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
docs: feedback additions
dab939a
to
6060279
Compare
Hey @marlonkeating, is it ready to review and merge? |
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.