Feat course optimizer page #1533
Conversation
| @@ -47,6 +48,7 @@ export default function initializeStore(preloadedState = undefined) { | |||
| processingNotification: processingNotificationReducer, | |||
| helpUrls: helpUrlsReducer, | |||
| courseExport: courseExportReducer, | |||
| courseOptimizer: courseOptimizerReducer, | |||
There was a problem hiding this comment.
Is there a reason for putting this in the Redux state and for implementing all this new code as JavaScript instead of TypeScript? I would really prefer that all net-new modules be using TypeScript, React Context, and React Query instead of JS and Redux, as recommended in OEP-67.
Is there something more I can do to raise awareness of this?
There was a problem hiding this comment.
Hi Braden, thank you so much for the feedback. I have thought this over. Let me explain where we are at.
I understand very well the wish to move us towards using TypeScript for new features. I completely agree, wholeheartedly. I was part of the fedx discussions around enabling more TypeScript.
We have very limited capacity and are looking at pushing this out fairly quickly.
We managed to base this largely on the "Export" feature that allowed us to quickly copy over code and adjust. That feature was not written in TypeScript in the first place.
I didn't succeed in using TypeScript partially for API handling here and it was taking too long to figure out.
We don't have experience with converting a lot of this functionality to TypeScript, even though I'm fluent at TypeScript. This would be a bit of a trailblazing effort but it would need quite an overhaul from the original code of the export we're basing this on.
When we were discussing whether or not to build this feature, we were working off of the promise to base this off of existing code of the export page. Needing to rewrite that in TypeScript was not something we considered and is changing the scope of this feature significantly. We also weren't aware that this would be an acceptance criteria to get a PR approved.
I'm willing to give it another try to change some very "core" functionality to TypeScript, around the API calls, but I think I would need some help with that if I need to do that? Would you be interested in doing an hour of pair programming on it sometime?
Otherwise I hope it's okay if we proceed as planned. I do think this is an important topic to factor in next time we propose a new feature. Maybe when we discuss the next feature proposal with the community we can point out the use of TypeScript from the get-go? So we can factor it in in our determination of cost vs. value.
Does that sound okay? What do you think?
There was a problem hiding this comment.
We also weren't aware that this would be an acceptance criteria to get a PR approved.
To be clear, I'm not going to try to block this PR until it's converted to TypeScript. It's fine as is, but in a perfect world it would have been written in TypeScript etc. in the first place.
We managed to base this largely on the "Export" feature that allowed us to quickly copy over code and adjust. That feature was not written in TypeScript in the first place.
I see; that does make sense. If you wrote it from scratch, it would not make sense to me, because IMHO it's easier to write new things using the new patterns than the existing patterns, though obviously there is a bit of a learning curve.
What I would ask is that next time you write any REST API related code in this repo, please use one of Library API, Taxonomy API, or Search API as a starting point, as those are already written using React Query and TypeScript.
And likewise for the actual page content and tests, this repo contains a huge mix of code styles, so if you can try to pick some of the newer ones (currently, any of the Libraries, Taxonomy, or Search UI) and any tests that are written using testUtils/initializeMocks, you'll be following the current best practices without any additional work.
I'm willing to give it another try to change some very "core" functionality to TypeScript, around the API calls, but I think I would need some help with that if I need to do that? Would you be interested in doing an hour of pair programming on it sometime?
Again, not a requirement, but - that would be great! See the examples I mentioned above and ping me on Slack anytime you want to do pair programming or have any questions I can answer.
There was a problem hiding this comment.
Also, you should know that you can put // @ts-check as the first line of your .jsx files, and you'll still get many of the benefits of using TypeScript in that file (e.g. code completion, type checking, and inline documentation in your IDE) without having to do the work of converting the file to TypeScript. It does make things stricter though, so sometimes you need to use JSDoc annotations and type casting to eliminate the warnings after making that change.
There are many examples in the codebase, but I'll point to ContentTagsDrawerHelper.jsx in particular as it is fully-typed and was merged in here before we even supported TypeScript at all. It has examples of importing types and doing type casting. But you can also see CardHeader.jsx which is an example of "turning on" type checking that required almost no other changes to the file.
There was a problem hiding this comment.
Thank you, this is extremely helpful! I'll start with moving the API calls to TS and spiraling out a bit from there, I just need to timebox it. I'll look into that code you shared and reach out to you if I have questions
| }; | ||
|
|
||
| CourseOptimizerPage.defaultProps = {}; | ||
| export default injectIntl(CourseOptimizerPage); |
There was a problem hiding this comment.
Please use useIntl instead of injectIntl. If you use VS Code there should be a visual reminder of this:
let me know if not.
| courseId: PropTypes.string.isRequired, | ||
| }; | ||
|
|
||
| CourseOptimizerPage.defaultProps = {}; |
There was a problem hiding this comment.
Don't use defaultProps - it's deprecated.
| CourseOptimizerPage.propTypes = { | ||
| intl: intlShape.isRequired, | ||
| courseId: PropTypes.string.isRequired, | ||
| }; |
There was a problem hiding this comment.
I strongly prefer TypeScript types over propTypes, because TypeScript types get checked by CI whereas propTypes warnings are routinely ignored. Open up the JS console on any authoring MFE page in your browser, and you'll see the problem.
src/optimizer-page/data/api.test.js
Outdated
| beforeEach(() => { | ||
| initializeMockApp({ | ||
| authenticatedUser: { | ||
| userId: 3, | ||
| username: 'abc123', | ||
| administrator: true, | ||
| roles: [], | ||
| }, | ||
| }); | ||
| axiosMock = new MockAdapter(getAuthenticatedHttpClient()); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Instead of all this boilerplate, you can just use
const { axiosMock } = initializeMocks();as the first line of each test case. You can then remove the beforeEach and afterEach you have here, because initializeMocks takes care of that all for you, including jest.clearAllMocks();.
See
frontend-app-authoring/src/testUtils.tsx
Lines 152 to 162 in f86c609
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1533 +/- ##
==========================================
- Coverage 93.08% 92.31% -0.77%
==========================================
Files 1051 1088 +37
Lines 20586 21418 +832
Branches 4459 4513 +54
==========================================
+ Hits 19162 19772 +610
- Misses 1355 1580 +225
+ Partials 69 66 -3 ☔ View full report in Codecov by Sentry. |
Description
Course Optimizer Page.
Depends on openedx/edx-platform#35887 - test together.
This also requires adding a nav menu item to edx-platform. That should be implemented and merged at the same time as this.
Rest of Description coming ASAP.