Upgraded frontend-build version #593

BilalQamar95 · 2022-08-04T09:18:16Z

Ticket:
42: Upgrade eslint to v8.x

What changed?

Updated frontend-build to v12 (Eslint was updated in frontend-build version resulting in it's version being bumped to v12. This PR updates frontend-build to reciprocate eslint version update)
Resolved eslint issues

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

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

…merge

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

adamstankiewicz · 2022-09-01T14:46:16Z

package.json

@@ -55,7 +55,7 @@
    "universal-cookie": "4.0.4"
  },
  "devDependencies": {
-    "@edx/frontend-build": "11.0.2",
+    "@edx/frontend-build": "^12.0.3",


nit: let's keep the version pinned like our other dependencies are.

adamstankiewicz · 2022-09-01T14:55:49Z

src/components/Toasts/ToastsProvider.jsx


  return (
-    <ToastsContext.Provider value={{ toasts, addToast, removeToast }}>
+    <ToastsContext.Provider value={
+      useMemo(() => ({ toasts, addToast, removeToast }), [removeToast, toasts])


useMemo shouldn't be called inline like this. Let's move it up above and store its output as a new variable contextValue instead.

adamstankiewicz · 2022-09-01T14:56:48Z

src/components/app/LoginRefresh.jsx

@@ -5,6 +5,7 @@ import { Container } from '@edx/paragon';
 import { LoadingSpinner } from '../loading-spinner';
 import { loginRefresh } from '../../utils/common';

+// eslint-disable-next-line react/prop-types


[curious] Why are we disabling this ESLint rule here versus defining the prop type for children?

adamstankiewicz · 2022-09-01T14:57:25Z

src/components/app/LoginRefresh.test.jsx

+function LoginRefreshWithContext({ roles = [] }) {
+  return (
+    <AppContext.Provider value={
+      useMemo(() => ({


Call useMemo above the return statement within the component to create a new variable contextValue instead.

adamstankiewicz · 2022-09-01T15:38:21Z

src/components/course/data/hooks.jsx

+  // eslint-disable-next-line react-hooks/exhaustive-deps
  const baseQueryParams = new URLSearchParams(location.search);


This ESLint warning is valid and IMO should not be ignored. On each re-render of the component consuming this React hook, baseQueryParams will be recomputed and have a new reference. By including it in the useMemo below, baseEnrollmentOptions is re-computed every single time since baseQueryParams changes each re-render. Should the baseQueryParams be computed with a useMemo as well?

adamstankiewicz · 2022-09-01T16:36:26Z

src/components/search/SearchCourseCard.jsx

  const { enterpriseConfig: { slug, uuid } } = useContext(AppContext);

+  // eslint-disable-next-line react-hooks/exhaustive-deps
  const course = hit ? camelCaseObject(hit) : {};


This is a valid ESLint error we shouldn't ignore. On every re-render of SearchCourseCard, the course variable be have a new reference, which will make linkToCourse re-compute on every re-render as well. Should we compute course with a useMemo?

adamstankiewicz · 2022-09-01T16:38:55Z

src/components/skills-quiz/SearchCurrentJobCard.jsx

+      }
+      return jobsArray;
+    },
+    [currentJob],


[curious] Is jobToFetch re-computed on every render of SearchCurrentJobCard? I believe currentJob in the dependency array will have a new variable reference on each re-render, and thus cause jobToFetch to unnecessarily re-compute each re-render.

Also nit: i think this variable might be intended to be named jobsToFetch since its a collection of multiple jobs?

@BilalQamar95 Bump on the first question around whether currentJob in the dependency array is causing this memo to constantly re-run since currentJob will have a new variable reference on every re-render of SearchCurrentJobCard.

I couldn't test it in the current app as it required making test data & going through the workflow which would take time but I do believe that you are right here. With currentJob in the dependency array, it seems that jobsToFetch will re-compute with each re-render.

adamstankiewicz · 2022-09-01T16:40:20Z

src/components/skills-quiz/SearchJobCard.jsx

+      }
+      return jobsArray;
+    },
+    [jobs],


[curious] Is jobsToFetch re-computed on every render of SearchJobCard ? I believe jobs in the dependency array will have a new variable reference on each re-render, and thus cause jobsToFetch to unnecessarily re-compute each re-render.

Bump on this question.

I do believe that you are right here, with jobs in the dependency array, it seems that jobsToFetch will re-compute with each re-render.

adamstankiewicz · 2022-09-01T16:41:01Z

src/components/skills-quiz/SkillsContextProvider.jsx

+  // eslint-disable-next-line react/jsx-no-constructed-context-values
  const value = { state, dispatch };


This is a valid ESLint error for a component. Context values should typically always be memoized.

adamstankiewicz · 2022-09-01T16:41:24Z

src/components/skills-quiz/SkillsQuizHeader.jsx

-      </h2>
+function SkillsQuizHeader() {
+  return (
+    <div style={{ display: 'flex' }} className="ml-2">


nit: the style prop is unnecessary here... use the d-flex class name instead.

adamstankiewicz

@BilalQamar95 Looking good. I just have 2 remaining questions bumped from the previous review :)

adamstankiewicz · 2022-09-07T14:53:32Z

src/components/skills-quiz/SearchCurrentJobCard.jsx

+      }
+      return jobsArray;
+    },
+    [currentJob],


@BilalQamar95 Bump on the first question around whether currentJob in the dependency array is causing this memo to constantly re-run since currentJob will have a new variable reference on every re-render of SearchCurrentJobCard.

adamstankiewicz · 2022-09-07T14:53:50Z

src/components/skills-quiz/SearchJobCard.jsx

+      }
+      return jobsArray;
+    },
+    [jobs],


Bump on this question.

…ues rule

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

codecov · 2022-10-28T07:21:50Z

Codecov Report

Base: 79.06% // Head: 79.15% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (3f4a0ad) compared to base (b35e2b8).
Patch coverage: 80.48% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   79.06%   79.15%   +0.09%     
==========================================
  Files         264      266       +2     
  Lines        5206     5229      +23     
  Branches     1280     1282       +2     
==========================================
+ Hits         4116     4139      +23     
+ Misses       1065     1064       -1     
- Partials       25       26       +1

Impacted Files	Coverage Δ
src/components/Toasts/ToastsProvider.jsx	`21.42% <0.00%> (-1.65%)`	⬇️
src/components/app/App.jsx	`0.00% <0.00%> (ø)`
src/components/app/EnterpriseAppPageRoutes.jsx	`0.00% <0.00%> (ø)`
src/components/course/CourseMainContent.jsx	`0.00% <0.00%> (ø)`
src/components/course/CoursePage.jsx	`0.00% <0.00%> (ø)`
src/components/course/CourseRunCard.jsx	`96.77% <ø> (ø)`
src/components/course/CourseSidebar.jsx	`0.00% <0.00%> (ø)`
src/components/course/CourseSidebarListItem.jsx	`0.00% <0.00%> (ø)`
src/components/course/CourseSidebarPrice.jsx	`97.61% <ø> (ø)`
src/components/course/EnrollModal.jsx	`93.10% <ø> (ø)`
... and 105 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

arbrandes · 2023-01-23T18:43:56Z

@BilalQamar95, have you addressed all of Adam's comments? Should we request a second pass? Maybe you could resolve the conflicts in the meantime.

BilalQamar95 · 2023-01-26T08:23:53Z

@arbrandes yes, Adam's comments have been addressed, however this PR is quite old now & upgrades the frontend-build to v12.0.3 resolving respective eslint issues whereas while resolving conflicts I realized the current frontend-build version of the repo is v12.4.15.

I'm closing this PR as it seems that this task is already complete with the merge of #660 which upgrades frontend-build to v12.4.15 & resolves all the subsequent eslint issues.

BilalQamar95 added 4 commits August 4, 2022 13:15

refactor: updated frontend-build version

c8e76ba

refactor: resolved eslint issues

86e15db

refactor: resolved eslint issues

9f917d8

refactor: resolved eslint issues

d5dd932

BilalQamar95 added the needs triage label Aug 4, 2022

BilalQamar95 requested a review from a team August 4, 2022 09:18

BilalQamar95 self-assigned this Aug 4, 2022

BilalQamar95 changed the title ~~Upgraded frontend-build version to v12~~ Upgraded frontend-build version Aug 4, 2022

BilalQamar95 added 5 commits August 16, 2022 15:43

Merge branch 'master' of https://github.com/openedx/frontend-app-lear…

698d52f

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

refactor: updated package-lock & resolved eslint after master branch …

1914ac9

…merge

refactor: updated files to ignore jsx-no-useless-fragment rule

67bc7d6

Merge branch 'master' of https://github.com/openedx/frontend-app-lear…

fbe59f5

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues post master branch merge

656ef07

BilalQamar95 removed the needs triage label Aug 25, 2022

abdullahwaheed added the engineering review label Aug 25, 2022

BilalQamar95 added 3 commits September 1, 2022 14:16

refactor: resolved merge conflicts with master

e478a32

refactor: resolved eslint issues after master branch merge

d31f1e3

refactor: resolved failing test cases

b0a3664

adamstankiewicz reviewed Sep 1, 2022

View reviewed changes

refactor: implemented suggested changes

b9f3e23

adamstankiewicz reviewed Sep 7, 2022

View reviewed changes

BilalQamar95 added 3 commits September 16, 2022 17:33

refactor: updated tests to accommodate jsx-no-constructed-context-val…

8c1b8b4

…ues rule

Merge branch 'master' of https://github.com/openedx/frontend-app-lear…

8f6bcfe

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

refactor: updated frontend-build & resolved eslint issues

9e4a052

arbrandes mentioned this pull request Nov 1, 2022

Upgrade eslint to v8.x openedx/wg-frontend#42

Closed

27 tasks

BilalQamar95 added 3 commits November 17, 2022 13:39

Merge branch 'master' of https://github.com/openedx/frontend-app-lear…

5dee7ad

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues

3063711

Merge branch 'master' of https://github.com/openedx/frontend-app-lear…

3f4a0ad

…ner-portal-enterprise into bilalqamar95/frontend-build-upgrade

BilalQamar95 closed this Jan 26, 2023

BilalQamar95 deleted the bilalqamar95/frontend-build-upgrade branch January 27, 2023 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgraded frontend-build version #593

Upgraded frontend-build version #593

BilalQamar95 commented Aug 4, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 8, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 8, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz Sep 1, 2022

adamstankiewicz left a comment

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

codecov bot commented Oct 28, 2022 •

edited

Loading

arbrandes commented Jan 23, 2023

BilalQamar95 commented Jan 26, 2023

		// eslint-disable-next-line react-hooks/exhaustive-deps
		const baseQueryParams = new URLSearchParams(location.search);

		// eslint-disable-next-line react/jsx-no-constructed-context-values
		const value = { state, dispatch };

Upgraded frontend-build version #593

Upgraded frontend-build version #593

Conversation

BilalQamar95 commented Aug 4, 2022

For all changes

Only if submitting a visual change

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

Choose a reason for hiding this 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

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

arbrandes commented Jan 23, 2023

BilalQamar95 commented Jan 26, 2023

codecov bot commented Oct 28, 2022 •

edited

Loading