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

docs: [OEP-67] TypeScript for static type checking #521

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

brian-smith-tcril
Copy link
Contributor

No description provided.

Comment on lines 41 to 42
TypeScript
==========
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched these over to sections so :ref: would work

Copy link

@muselesscreator muselesscreator left a comment

Choose a reason for hiding this comment

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

Yees

@brian-smith-tcril brian-smith-tcril marked this pull request as ready for review October 2, 2023 15:42
==========

Several variants of typed JavaScript have grown in community popularity
recently, with `TypeScript`_ as the most popular. edX believes adding
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
recently, with `TypeScript`_ as the most popular. edX believes adding
recently, with `TypeScript`_ as the most popular. The Open edX community believes adding

But really, should this section be rewritten since you're adding an ADR about actually using TypeScript? It's ok to update an OEP, just the update should be recorded in the change history at the bottom

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought this was the OEP file not an ADR file. I would make the suggested language change but keep the updated TypeScript recommendation in ADR 7.

********

Frontend repositories within the Open edX project will be updated to support the use of TypeScript, without
requiring existing frontend JavaScript code be rewritten in TypeScript.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this (the Decision & Consequence sections) I lack context as to why this decision has been made and what consequence it has (like, what's the benefit of using TypeScript). I know you link out to external resources but I think it could be beneficial to add a sentence or two to provide context without requiring reading through external resources as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a quick summary of a couple benefits of typescript here https://github.com/openedx/open-edx-proposals/compare/3af815b84c789af807d4e42927938024557c669f..f71d62cd46797a78be384e73f6577913ea599014

Let me know if there's something you feel is still missing!

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it, thank you!

@brian-smith-tcril brian-smith-tcril changed the title docs: [OEP-11] TypeScript for static type checking docs: [OEP-67] TypeScript for static type checking Oct 26, 2023
@feanil feanil merged commit 66eb2ab into openedx:master Dec 12, 2023
2 checks passed
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.

4 participants