Upgraded frontend-build version to v12 #836

BilalQamar95 · 2022-07-18T11:30:42Z

Git 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

…n-portal into bilalqamar95/frontend-build-upgrade

codecov · 2022-07-18T12:03:50Z

Codecov Report

Base: 82.43% // Head: 83.17% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (83eb04b) compared to base (bb0c728).
Patch coverage: 84.14% of modified lines in pull request are covered.

❗ Current head 83eb04b differs from pull request most recent head d6b9112. Consider uploading reports for the commit d6b9112 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #836      +/-   ##
==========================================
+ Coverage   82.43%   83.17%   +0.73%     
==========================================
  Files         398      386      -12     
  Lines        8989     8510     -479     
  Branches     1895     1828      -67     
==========================================
- Hits         7410     7078     -332     
+ Misses       1546     1400     -146     
+ Partials       33       32       -1

Impacted Files	Coverage Δ
src/components/App/index.jsx	`0.00% <0.00%> (ø)`
...rc/components/AuthenticatedEnterpriseApp/index.jsx	`0.00% <0.00%> (ø)`
...mponents/BulkEnrollmentPage/stepper/ReviewStep.jsx	`50.00% <0.00%> (-10.00%)`	⬇️
src/components/CodeManagement/index.jsx	`0.00% <0.00%> (-50.00%)`	⬇️
src/components/CodeSearchResults/index.jsx	`84.37% <ø> (ø)`
...mponents/ContentHighlights/ContentHighlightSet.jsx	`0.00% <0.00%> (-25.00%)`	⬇️
...entHighlights/ContentHighlightSetCardContainer.jsx	`0.00% <0.00%> (ø)`
src/components/ContentHighlights/data/reducer.js	`0.00% <0.00%> (ø)`
src/components/EnterpriseList/index.jsx	`80.70% <0.00%> (ø)`
src/components/FileInput/index.jsx	`35.00% <0.00%> (ø)`
... and 329 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.

…n-portal into bilalqamar95/frontend-build-upgrade

…context-values for tests

adamstankiewicz · 2022-08-17T12:44:07Z

.eslintrc

+    "react/no-unstable-nested-components": [
+      2, { "allowAsProps": true }
+    ],


Hmm, I don't think we should change this rule as it tries to prevent creating potentially unstable components, when React components are defined inline with another component, which indeed does feel like an anti-pattern.

Could we abstract these inline components as a completely standalone component instead?

adamstankiewicz · 2022-08-17T12:46:03Z

.eslintrc

@@ -1,6 +1,6 @@
 {
  "extends": "@edx/eslint-config",
-  "parser": "babel-eslint",
+  "parser": "@babel/eslint-parser",


FWIW, ideally this repo (or most MFE repos) wouldn't need a custom .eslintrc file. Most of the stuff in this file is legacy from when this repo was first created in 2018, before @edx/frontend-build existed.

OK to keep for now, but I'd love to see this repo move away from a custom .eslintrc file in favor of using the default .eslintrc config from @edx/frontend-build.

adamstankiewicz · 2022-08-17T12:51:22Z

src/components/BulkEnrollmentPage/stepper/AddCoursesStep.test.jsx

    subscription: [{}, () => {}],
-  };
+  }), [selectedCourses]);


Two things here:

Technically, selectedCourses will have a new array reference on each re-render of StepperWrapper, essentially getting re-created each time. This could cause value to change indefinitely instead of being memoized as intended. Either selectedCourses should be computed inside the useMemo or the selectedCourses itself should be memoized.

Why was selectedEmails replaced with an empty array here?

This was covered in one of the previous commits, made in light of changes suggested by you on this PR.

adamstankiewicz · 2022-08-17T12:52:56Z

src/components/BulkEnrollmentPage/stepper/BulkEnrollmentSubmit.test.jsx

-);
+function BulkEnrollmentSubmitWrapper({ bulkEnrollInfo = defaultBulkEnrollInfo, ...props }) {
+  return (
+    <ToastsContext.Provider value={useMemo(() => ({ addToast }), [])}>


nit: abstract the useMemo into a variable contextValue instead of defining it inline.

This was covered in one of the previous commits, made in light of changes suggested by you on this PR.

adamstankiewicz · 2022-08-17T12:53:52Z

src/components/BulkEnrollmentResultsDownloadPage/BulkEnrollmentResultsDownloadPage.test.jsx

+  return (
+    <Provider store={mockStore({ portalConfiguration: { enterpriseId: '1234' } })}>
+      <Router history={history}>
+        <ToastsContext.Provider value={useMemo(() => ({ addToast: mockAddToast }), [])}>


nit: abstract the useMemo out of the return statement into its own variable contextValue.

This was covered in one of the previous commits, made in light of changes suggested by you on this PR.

adamstankiewicz · 2022-09-07T14:35:30Z

src/components/Toasts/ToastsProvider.jsx

@@ -26,11 +26,12 @@ const ToastsProvider = ({ children }) => {
  };

  return (
+    // eslint-disable-next-line react/jsx-no-constructed-context-values


Should this context value be computed with a useMemo to avoid ignoring this ESLint rule?

adamstankiewicz · 2022-09-07T14:39:00Z

src/components/settings/SettingsSSOTab/ExistingSSOConfigs.jsx

-                  <div className="justify-content-center">
-                    {config.display_name}
-                  </div>
+    <span>


nit: unrelated to your change, but we're using block elements (p) inside of inline elements (span) which is typically an anti-pattern / not best practice. Any reason not to change the span to be div here?

adamstankiewicz · 2022-09-07T14:41:06Z

src/containers/EnterpriseApp/EnterpriseApp.test.jsx

@@ -18,6 +18,7 @@ import { features } from '../../config';
 import NotFoundPage from '../../components/NotFoundPage';
 import { EnterpriseSubsidiesContext } from '../../components/EnterpriseSubsidiesContext';

+// eslint-disable-next-line react/function-component-definition


nit: Why is this rule disabled here but in other test files, you've replaced arrow function components with function?

Replacing it here results in following error,
The module factory of jest.mock() is not allowed to reference any out-of-scope variables.

adamstankiewicz · 2022-09-07T14:44:10Z

src/components/subscriptions/data/hooks.js

@@ -69,6 +69,7 @@ export const useSubscriptions = ({ enterpriseId, errors, setErrors }) => {
    loadCustomerAgreementData();
  }, [loadCustomerAgreementData]);

+  // eslint-disable-next-line react-hooks/exhaustive-deps
  useEffect(loadCustomerAgreementData, [enterpriseId]);


We should avoid ignoring ESLint rules about react-hooks/exhaustive-deps. Is there a fix here to avoid getting the error?

That would be to replace enterpriseId with loadCustomerAgreementData in dependency array. When it comes to useEffect, I generally avoid making these changes just to satisfy the linter

adamstankiewicz · 2022-09-07T14:46:19Z

src/components/settings/data/hooks.js

@@ -47,6 +47,7 @@ export const useLinkManagement = (enterpriseUUID) => {
    };
  }, [enterpriseUUID]);

+  // eslint-disable-next-line react-hooks/exhaustive-deps
  useEffect(loadLinks, []);


We should generally avoid ignoring ESLint rules about react-hooks/exhaustive-deps. Is there a solution here to avoid needing to disable this rule?

That would be to either add loadLinks to the dependency array or remove the array entirely. I have seen these being ignored for useEffect in other areas/repos as well. One such example would be in SSOConfigConfiguredCard.jsx

…rule

…portal into bilalqamar95/frontend-build-upgrade

…n-portal into bilalqamar95/frontend-build-upgrade

…portal into bilalqamar95/frontend-build-upgrade

BilalQamar95 · 2022-12-29T12:03:15Z

PR upgrading frontend-build to v12 was already merged, therefore I'm closing this on

refactor: upgraded frontend-build version to v12

672f22b

BilalQamar95 requested a review from a team July 18, 2022 11:30

BilalQamar95 self-assigned this Jul 18, 2022

BilalQamar95 added 2 commits July 18, 2022 16:44

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

3a81efd

…n-portal into bilalqamar95/frontend-build-upgrade

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

566ac23

…n-portal into bilalqamar95/frontend-build-upgrade

BilalQamar95 closed this Jul 20, 2022

BilalQamar95 added 4 commits July 27, 2022 14:03

refactor: updated packages

59553fa

refactor: resolved eslint issues

0a10a03

refactor: resolved eslint issues

29bad49

refactor: resolved eslint issues

0210f6b

BilalQamar95 reopened this Jul 28, 2022

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

4d96449

…n-portal into bilalqamar95/frontend-build-upgrade

BilalQamar95 added the needs triage label Jul 28, 2022

BilalQamar95 added 3 commits August 10, 2022 15:33

refactor: ignored react-hooks eslint exhaustive-deps issues

5fd52f9

refactor: ignored jsx-no-useless-fragment rule for specific cases

1a054b0

refactor: updated fronend-build to v12.0.3

d6a8590

BilalQamar95 removed the needs triage label Aug 25, 2022

abdullahwaheed added the engineering review label Aug 25, 2022

BilalQamar95 added 3 commits September 7, 2022 15:52

refactor: pinned frontend-build version & ignored jsx-no-constructed-…

6f714e0

…context-values for tests

refactor: merged with master branch

4c6e63e

refactor: resolved eslint issues post master merge

bded191

adamstankiewicz reviewed Sep 7, 2022

View reviewed changes

BilalQamar95 added 4 commits September 8, 2022 16:10

refactor: made requested changes

95be78a

refactor: updated tests to satisfy jsx-no-constructed-context-values …

e42481a

…rule

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

e9e1d86

…portal into bilalqamar95/frontend-build-upgrade

refactor: updated frontend-build & resolved eslint issues

2820bda

arbrandes mentioned this pull request Nov 1, 2022

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

Closed

27 tasks

BilalQamar95 added 2 commits November 9, 2022 17:06

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

381d00d

…portal into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues post master branch merge

6c30b1e

BilalQamar95 added 6 commits November 17, 2022 16:59

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

c6f0dac

…portal into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues post master merge

57228c5

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

1e9294d

…portal into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues post master merge

cf6ffe7

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

83eb04b

…n-portal into bilalqamar95/frontend-build-upgrade

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

d6b9112

…portal into bilalqamar95/frontend-build-upgrade

BilalQamar95 closed this Dec 29, 2022

abdullahwaheed removed the engineering review label Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgraded frontend-build version to v12 #836

Upgraded frontend-build version to v12 #836

BilalQamar95 commented Jul 18, 2022 •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading

adamstankiewicz Aug 17, 2022

adamstankiewicz Aug 17, 2022

adamstankiewicz Aug 17, 2022

BilalQamar95 Sep 8, 2022

adamstankiewicz Aug 17, 2022

BilalQamar95 Sep 8, 2022

adamstankiewicz Aug 17, 2022

BilalQamar95 Sep 8, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 8, 2022 •

edited

Loading

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 8, 2022 •

edited

Loading

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 8, 2022

BilalQamar95 commented Dec 29, 2022

Upgraded frontend-build version to v12 #836

Upgraded frontend-build version to v12 #836

Conversation

BilalQamar95 commented Jul 18, 2022 • edited Loading

For all changes

Only if submitting a visual change

codecov bot commented Jul 18, 2022 • edited Loading

Codecov Report

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

BilalQamar95 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BilalQamar95 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BilalQamar95 commented Dec 29, 2022

BilalQamar95 commented Jul 18, 2022 •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading

BilalQamar95 Sep 8, 2022 •

edited

Loading

BilalQamar95 Sep 8, 2022 •

edited

Loading