Add typescript support and migrate function example #782
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #782 +/- ##
==========================================
- Coverage 84.87% 84.83% -0.04%
==========================================
Files 320 322 +2
Lines 6399 6521 +122
Branches 1552 1578 +26
==========================================
+ Hits 5431 5532 +101
- Misses 941 960 +19
- Partials 27 29 +2
☔ View full report in Codecov by Sentry. |
.eslintignore
Outdated
| src/segment.jsx/ | ||
| utils.jsx |
There was a problem hiding this comment.
For some reason, course/data/utils.jsx and only this file says that it is not included under the tsconfig file.
There was a problem hiding this comment.
Renaming utils.tsx fixed the issue. Props to @davidjoy for suggesting this might be the issue!
| @@ -15,7 +15,7 @@ MenuTrigger.defaultProps = { | |||
| tag: 'div', | |||
| className: null, | |||
| }; | |||
| const MenuTriggerType = <MenuTrigger />.type; | |||
| const MenuTriggerType = (<MenuTrigger />).type; | |||
There was a problem hiding this comment.
typescript-eslint called this out while the old configuration didn't, for some reason.
src/components/course/data/utils.jsx
Outdated
| const startDate = new Date(start); | ||
| return startDate && today >= startDate; | ||
| } | ||
| import { compareRedeemableOffers, hasCourseStarted as hasCourseStartedTs} from './utils.tsx'; |
There was a problem hiding this comment.
Is it actually necessary to add the extensions now? I wonder if we could tweak the webpack config in frontend-build to avoid this. I know I have a (personal) TS project where I don't have to include extensions on my imports. 🤔
There was a problem hiding this comment.
If we don't have to disambiguate between TypeScript and a Javascript file with identical file paths, then we don't need to include a filename extension.
| @@ -88,8 +88,9 @@ | |||
| "build": "fedx-scripts webpack", | |||
| "build:with-theme": "THEME=npm:@edx/brand-edx.org@latest npm run install-theme && fedx-scripts webpack", | |||
| "i18n_extract": "BABEL_ENV=i18n fedx-scripts babel src --quiet > /dev/null", | |||
| "lint": "fedx-scripts eslint --ext .js --ext .jsx .", | |||
| "lint:fix": "fedx-scripts eslint --fix --ext .js --ext .jsx .", | |||
| "check-types": "tsc --noemit", | |||
There was a problem hiding this comment.
Wonder if we could delegate this to fedx-scripts like most of the other scripts here. It makes it easier to make changes across MFEs if the details of the build change in frontend-build.
|
|
||
| ## Decision | ||
|
|
||
| We will prioritize using TypeScript in the following places: |
There was a problem hiding this comment.
Two thoughts:
We already have support for typescript in frontend-build; I don't see a corresponding ADR there. I like the idea of moving this ADR into frontend-build to explain why we added typescript after advocating against it for a number of years. We've already made the choice to go for it more systemically, adding typescript to this repo may not even be ADR worthy unless there are some specific reasons we want to be early adopters for this repo.
Related to this specific "Decision" section - My feedback here is that I've found the most utility in using typescript in data/API/state layers, and it feels less useful in React components because it tends to get really complain-y and requires you to annotate all the internals of React for very little benefit. I assume most of our react components are already using PropTypes, too, which gives us some level of safety/checking there. If it were me I might write some of that nuance into this ADR.
There was a problem hiding this comment.
Created ADR in openedx/frontend-build#412
|
In general, exciting to see Typescript getting added to our toolbelt. I'm somewhat opinionated about using it responsibly, and feel like we might benefit from an ADR about it outside this repo. I doubt we'll all be able to agree on "when to TS", but having a statement in frontend-build about the fact that we're using it at all now would be good. |
build: rename ts file to fix duplicate js/ts filename issues
marlonkeating
force-pushed
the
mkeating/ENT-6897/1
branch
from
807433e
to
a6a3665
Compare
Jira Ticket
This change adds TypeScript support to the code base, and gives a case study in migrating Javascript to TypeScript one function at a time.
For all changes
Only if submitting a visual change