build: Add typescript support #703

marlonkeating · 2023-04-04T06:06:23Z

Jira Ticket

This change adds build support for TypeScript, plus several files converted to TypeScript.

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

johnnagro

⛵

adamstankiewicz

A couple things:

I'm a bit concerned at the amount of formatting/ESLint-type changes throughout the PR. Are these changes a potential code smell that the ESLint config isn't set up to support TypeScript (e.g., requiring a .ts(x) file extension on imports, splitting already reasonably short lines into multiple)?
The changes for TypeScript previously introduced in @edx/frontend-build v12.5.0 (installed in this PR). was since reverted in v12.6.2. As such, it feels risky to depend on v12.5.0 as when @edx/frontend-build gets upgraded beyond v12.6.2, we'd lose the TS changes that are now reverted.
- Note @edx/frontend-build is now getting installed with a ^, which means the installed version of the package might not actually match the explicit version defined in the package.json file (though it'll match the defined semantic version range). For example, running npm i on your branch, I get v12.6.1 of @edx/frontend-build installed despite ^12.5.0 listed in the package.json file.

src/components/course/CourseAssociatedPrograms.jsx

package.json

src/components/course/data/hooks.tsx

adamstankiewicz · 2023-04-05T13:11:31Z

src/components/course/data/service.ts

+          allRecommendations.length < 1 &&
+          samePartnerRecommendations.length < 1


[curious] What is causing all these unnecessary splitting of lines into multiple lines throughout this file and PR? In many instances, the original lines are well under the max line length of 120 per the ESLint config specified in @edx/frontend-build, but they are getting split into multiple lines? Not trying to be nitpicky on formatting, but these formatting changes feel a bit erroneous.

tsconfig.json

package.json

src/components/course/data/hooks.tsx

adamstankiewicz · 2023-04-18T14:28:32Z

src/components/course/data/hooks.tsx

+      return `${
+        config.LMS_BASE_URL
+      }/enterprise/grant_data_sharing_permissions/?${queryParams.toString()}`;
+    }


nit: I realize many of the formatting changes like this one are generated by your use of Prettier; that said, I disagree with Prettier's opinion here. Changes like this to go from 1 to 3 lines is unnecessary given our ESLint config allows lines up to 120 characters.

build: Use updated frontend-build typescript config chore: Merge from master and reconcile with new typescript lint config chore: Remove unnecessary typescript packages and fix tsconfig formatting

adamstankiewicz · 2023-04-19T21:31:52Z

.env.development

@@ -45,7 +45,7 @@ FEATURE_ENABLE_AUTO_APPLIED_LICENSES='true'
 LEARNING_TYPE_FACET='true'
 LEARNER_SUPPORT_URL='https://support.edx.org/hc/en-us'
 FEATURE_ENABLE_PATHWAYS='true'
-FEATURE_ENABLE_COURSE_REVIEW='true'
+FEATURE_ENABLE_COURSE_REVIEW=''


[curious] Is this change intentional/necessary? Just wondering if changing this from true to false might cause confusion for the Markhors team who may expect the feature to be enabled locally.

adamstankiewicz · 2023-04-19T21:37:29Z

src/components/course/CourseHeader.jsx

@@ -99,7 +99,7 @@ const CourseHeader = () => {
          <Col xs={12} lg={12}>
            {catalog.containsContentItems ? (
              <>
-                {features.ENABLE_COURSE_REVIEW && <CourseReview />}
+                {enableReviewSection && <CourseReview />}


This should be changed back to the original, relying on the feature flag.

adamstankiewicz · 2023-04-19T21:37:57Z

src/components/course/CourseRunCard.jsx

-  // Only users buckted in `Variation 1` can see the change.
-  const isExperimentVariation1 = isExperimentVariant(config.EXPERIMENT_2_ID, config.EXPERIMENT_2_VARIANT_1_ID);
-  // For our experiment, we should trigger our Optimizely event only when this condition is true
-  const triggerLicenseSubsidyEvent = shouldShowLicenseSubsidyPriceText;


We probably shouldn't be removing these? 🤔

adamstankiewicz · 2023-04-19T21:50:46Z

src/components/course/enrollment/EnrollAction.jsx

@@ -36,40 +36,38 @@ const EnrollAction = ({
  userEnrollment,
  subscriptionLicense,
  courseRunPrice,
-  triggerLicenseSubsidyEvent,


Why was this prop removed?

adamstankiewicz · 2023-04-19T21:51:35Z

src/components/course/enrollment/components/ToDataSharingConsent.jsx

@@ -47,7 +42,6 @@ const ToDataSharingConsentPage = ({ enrollLabel, enrollmentUrl, triggerLicenseSu
      onClick={(e) => {
        analyticsHandler(e);
        optimizelyHandler(e);
-        if (triggerLicenseSubsidyEvent) { optimizelyLicenseSubsidyHandler(e); }


[curious] Why is this logic getting removed?

adamstankiewicz · 2023-04-19T21:55:34Z

src/components/search/SearchCourseCard.jsx

  const isShortLengthCourse = isShortCourse(course);

  const primaryPartnerLogo = getPrimaryPartnerLogo(partnerDetails);

-  const handleCourseAboutPageVisitClick = useCourseAboutPageVisitClickHandler({


Why is this getting removed?

adamstankiewicz · 2023-04-19T21:55:59Z

src/components/search/data/hooks.js

- *
- * @returns Click handler function for course about page visit click events.
- */
-export const useCourseAboutPageVisitClickHandler = ({ href, courseKey, enterpriseId }) => {


Why is this getting removed?

adamstankiewicz · 2023-04-19T21:57:09Z

globals.d.ts

+    const content: string;
+    export default content;
+  }


nit: indenting is off.

adamstankiewicz · 2023-04-19T22:02:18Z

src/components/course/data/service.ts

    return this.cachedAuthenticatedHttpClient.get(url);
  }

-  fetchUserLicenseSubsidy(courseKey = this.activeCourseRun.key) {
+  fetchUserLicenseSubsidy(courseKey = this?.activeCourseRun?.key) {


[curious] When would this be undefined/null where it requires a ??

adamstankiewicz · 2023-04-19T22:03:15Z

src/components/course/data/utils.tsx

@@ -1,4 +1,5 @@
 import React from 'react';
+import isNumber from 'lodash.isnumber';


Nice refactor to bring this library in.

johnnagro approved these changes Apr 4, 2023

View reviewed changes

pwnage101 approved these changes Apr 4, 2023

View reviewed changes

adamstankiewicz reviewed Apr 5, 2023

View reviewed changes

marlonkeating force-pushed the mkeating/ENT-6897/0 branch from 85d3812 to 67925e2 Compare April 17, 2023 14:20

adamstankiewicz reviewed Apr 18, 2023

View reviewed changes

marlonkeating force-pushed the mkeating/ENT-6897/0 branch from 67925e2 to 8785aa2 Compare April 19, 2023 18:32

build: Add typescript support

c9e8c26

build: Use updated frontend-build typescript config chore: Merge from master and reconcile with new typescript lint config chore: Remove unnecessary typescript packages and fix tsconfig formatting

marlonkeating force-pushed the mkeating/ENT-6897/0 branch from 8785aa2 to c9e8c26 Compare April 19, 2023 21:56

adamstankiewicz reviewed Apr 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Add typescript support #703

build: Add typescript support #703

marlonkeating commented Apr 4, 2023

johnnagro left a comment

adamstankiewicz left a comment

adamstankiewicz Apr 5, 2023

adamstankiewicz Apr 18, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

adamstankiewicz Apr 19, 2023

		allRecommendations.length < 1 &&
		samePartnerRecommendations.length < 1

		@@ -1,4 +1,5 @@
		import React from 'react';
		import isNumber from 'lodash.isnumber';

build: Add typescript support #703

Are you sure you want to change the base?

build: Add typescript support #703

Conversation

marlonkeating commented Apr 4, 2023

For all changes

Only if submitting a visual change

johnnagro left a comment

Choose a reason for hiding this comment

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment