feat: replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace #1192

BilalQamar95 · 2024-03-29T12:21:48Z

Description

Replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace
Major version upgrades, Updating frontend-build to v14 & frontend-platform to v8 along with respective edx packages
Updated ts-jest to v29 along with respective packages
Fixed failing tests post upgrade by making necessary upgrades

…nedx namespace

codecov · 2024-03-29T14:16:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.39%. Comparing base (9f442ff) to head (127bb44).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1192   +/-   ##
=======================================
  Coverage   85.39%   85.39%           
=======================================
  Files         537      537           
  Lines       11684    11684           
  Branches     2430     2466   +36     
=======================================
  Hits         9977     9977           
+ Misses       1657     1656    -1     
- Partials       50       51    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adamstankiewicz · 2024-03-29T14:48:22Z

package.json

@@ -15,7 +15,6 @@
    "lint:fix": "fedx-scripts eslint --fix --ext .js --ext .jsx --ext .tsx --ext .ts .",
    "precommit": "npm run lint",
    "prepublishOnly": "npm run build",
-    "postinstall": "patch-package",


nit: I believe you could leave this command here (and keep patch-package installed), but continue to remove the @edx+paragon... patch from the patches directory at this point so when it runs, no patches are applied. Related, would recommend keeping the patches directory committed, even if empty with a .gitkeep file.

Rationale: leaves the option open to use patch-package again in the future if needed without re-configuring the removed lines like postinstall here. Leaving it as is with an empty patches directory shouldn't really have any downsides.

adamstankiewicz · 2024-03-29T14:50:53Z

package.json

+  "overrides": {
+    "@edx/frontend-platform": {
+      "@openedx/frontend-build": "13.1.0-alpha.2"
+    }


nit: while not a blocker per se, I would have liked to see @edx/frontend-platform's peer dependency for @openedx/frontend-build continue to support the alpha release of frontend-build like it used to before the @edx -> @openedx migration, which would prevent the need for maintaining an overrides in the MFE itself (e.g., this override would also be necessary in the other MFE that uses the alpha version of frontend-build).

openedx/frontend-platform@88fd140#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L77

With the overrides here, when alpha eventually merges into master, we will have to remember to remove the override here, which would otherwise be prevented if the upstream peer dependency didn't lose the alpha version...

i don't this we should add alpha as peer dependency since alpha is supposed to be a pre-release. We should keep peer dependencies for actual releases

See the above comment thread about how frontend-build's alpha was merged into master. We shouldn't need to rely on anything to do with alpha from frontend-build at this point.

adamstankiewicz · 2024-03-29T14:59:50Z

package.json

    "@edx/openedx-atlas": "^0.6.0",
-    "@edx/paragon": "20.46.3",
+    "@openedx/paragon": "21.11.4",


Please verify you are accounting for all breaking changes in these upgrades. For example, going from v20 -> v21 of Paragon is a breaking change for the SelectableBox.Set component for a11y, but I don't see the breaking change addressed in your PR (release notes).

SelectableBox.Set is used within LMSSelectorPage in this MFE. It should be using the ariaLabelledby prop to account for the breaking change and fulfill a11y requirements.

adamstankiewicz · 2024-03-29T15:05:47Z

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

+        if (value.includes('waiting')) {
+          const waitingForLearnerOption = filtersDropdownContainer.getByLabelText('Waiting for learner 1', { exact: false });
+          expect(waitingForLearnerOption).toBeInTheDocument();
+          userEvent.click(waitingForLearnerOption);


userEvent.click should not be used within an act. See https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#wrapping-things-in-act-unnecessarily for more details.

Since tests were failing without it act was used initially to ensure that all updates to the component (like state changes and DOM updates) were flushed and applied before assertions without these test was failing. However you are right, I have replaced it with waitFor which automatically handles the flushing of updates within its callback, waiting for the expected condition to be true which seems to have resolved the issue.

…build

…ps://github.com/openedx/frontend-app-admin-portal into bilalqamar95/replace-edx/paragon-frontend-build

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

adamstankiewicz · 2024-05-20T14:50:41Z

package.json

    "@edx/openedx-atlas": "^0.6.0",
-    "@edx/paragon": "20.46.3",
+    "@openedx/paragon": "21.11.4",


This PR is removing the (temporary) Paragon patch defined in the patches directory but we should verify the new Paragon version we upgrade to actually contains the fixes previously implemented in the Paragon patch.

Patch for DataTable's incorrect "Select all" row count is fixed in v21.12.0. This PR is only upgrading to v21.11.4, though, so by removing the patch and upgrading to a lesser version would reintroduce a bug regression.

Patch for the ManageAccounts Paragon icon to fix its fill color is fixed in v21.10.2, so this PR's upgrade would pull in the fix as expected for this patch.

Given (1) above, we should ensure we are installing the latest v21 version available to bring in the bug fix for DataTable's patch in v21.12.0+.

[inform] Note: Paragon v22 exists as the latest version; however, there are 4 components with breaking changes that may need to be addressed as well as upgrading the @edx/frontend-enterprise-* NPM packages to support Paragon v22 as a peer dependency. That said, I think this should PR should continue to only upgrade to v21 for the time being (defer on the v22 upgrade for now).

adamstankiewicz · 2024-05-20T14:55:56Z

package.json

@@ -91,7 +91,7 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.0.0",
-    "@edx/frontend-build": "^12.9.0-alpha.1",
+    "@openedx/frontend-build": "13.1.0-alpha.2",


[inform] The alpha version was previously used due to this MFE's usage of TypeScript before the TypeScript support was merged into master for @openedx/frontend-build. However, as of v14 in @openedx/frontend-build, the TypeScript support has been merged into master (per this GitHub release).

As a result, this repo should be able to consume the latest distribution channel instead of alpha at this point.

This was intentional as v14 contains jest v29 upgrade & this PR focuses on @edx/paragon & @edx/frontend-build to openedx namespace switch. There is another PR (currently drafted) for jest v29 upgrade, containing upgrade to frontend-build v14 along with respective edx packages, which will be updated and ready for review once this PR is merged

I'm not really following why this work needs to be split into 2 separate PRs at this point. Why can't the upgrades from the draft PR just be migrated to this PR since they are so related anyways? IMHO, I would have considered Jest v29 upgrade (and its other Jest-related package upgrades) as part of the original scope of migrating to latest version of @openedx/frontend-build in this PR directly; personally don't really see why this work needs to be split across 2 PRs.

Either way, I'm not going to push back further if you feel the incremental upgrade and separate upgrade is truly necessary versus bringing the other upgrades into this PR (given Jest upgrade, etc. actually is related to the scope of migrating to @openedx/frontend-build, IMHO).

Thank you for your feedback. My rationale behind splitting the work into 2 different PRs was based on

Scope and Complexity Management: The primary focus of the current PR is the namespace switch to @openedx for @edx/paragon and @edx/frontend-build. This was scoped as a standalone epic to ensure a clean transition without introducing additional variables. Also the frontend-build v14 brings in several major package upgrades further adding to the complexity which when handled separately could ensure a more focused and manageable review process.

Project Timeline and Dependencies: When the PR was initially created, frontend-build v14 was not yet available. It was intended to address the namespace switch only. The subsequent release of frontend-build v14 necessitated additional work, which was scoped into a separate Jest v29 epic

Given the significant number of file changes and potential issues arising from multiple major upgrades, splitting the work allows for easier tracking and isolation of issues, also incremental approach reduces the risk of introducing widespread breaking changes and simplifies troubleshooting if things go south.

The upgrades correspond to different epics, one focused on the namespace switch and the other on upgrading Jest and related packages. Keeping them separate maintains alignment with the objectives.

I understand the perspective that combining the upgrades might seem more efficient. I have decided to combine the two considering the review process duration and the fact that the change itself, while widespread, is not overly complicated, it will ultimately expedite the merge process and reduce the overall time required for reviews by having single review for both features.

adamstankiewicz · 2024-05-20T14:57:26Z

package.json

+  "overrides": {
+    "@edx/frontend-platform": {
+      "@openedx/frontend-build": "13.1.0-alpha.2"
+    }


See the above comment thread about how frontend-build's alpha was merged into master. We shouldn't need to rely on anything to do with alpha from frontend-build at this point.

src/components/settings/SettingsLMSTab/LMSSelectorPage.tsx

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

…ctive packages

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

adamstankiewicz

LGTM (with tiny nit around pinning the Paragon dependency). Thanks for addressing the feedback, @BilalQamar95. Happy to help QA/verify on stage once merged.

adamstankiewicz · 2024-05-22T12:27:22Z

package.json

    "@edx/openedx-atlas": "^0.6.0",
-    "@edx/paragon": "20.46.3",
+    "@openedx/paragon": "^21.13.1",


nit: pin dependency

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

BilalQamar95 · 2024-05-22T13:11:16Z

LGTM (with tiny nit around pinning the Paragon dependency). Thanks for addressing the feedback, @BilalQamar95. Happy to help QA/verify on stage once merged.

Thank you for the approval & for QA/verification on stage. I have pinned the paragon version and it will be merged once CI clears

BilalQamar95 added 3 commits March 29, 2024 14:59

feat: replaced packages @edx/paragon & @edx/frontend-build to use ope…

671c474

…nedx namespace

refactor: updated snapshots for failing tests

93e0bfb

refactor: resolved failing tests

56d2d2f

BilalQamar95 self-assigned this Mar 29, 2024

refactor: removed patch-package

2dc6d95

adamstankiewicz reviewed Mar 29, 2024

View reviewed changes

BilalQamar95 added 7 commits April 1, 2024 15:43

refactor: updated budgetDetailPage test

cc3f22d

refactor: added ariaLabelledby prop to SelectableBox

63d7b1a

refactor: re-installed patch-package

2a8f938

Merge branch 'master' into bilalqamar95/replace-edx/paragon-frontend-…

bbf9d8a

…build

Merge branch 'bilalqamar95/replace-edx/paragon-frontend-build' of htt…

8faaab1

…ps://github.com/openedx/frontend-app-admin-portal into bilalqamar95/replace-edx/paragon-frontend-build

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

464581f

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

refactor: updated paragon imports to openedx

5bc72ec

adamstankiewicz reviewed May 20, 2024

View reviewed changes

BilalQamar95 added 8 commits May 21, 2024 15:15

feat: bump paragon version, update snapshot

9375114

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

39dcb4a

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

refactor: updated LMS selector page SelectableBox.Set arialabel

455e5c9

refactor: updated LMSSelectorPage p tag id

4704ece

feat: updated frontend-build to v14 & ts-jest to v29 along with respe…

06102e0

…ctive packages

fix: updated snapshots for failing tests

6d1464b

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

ddf2dff

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

fix: fixed failing tests

c890afc

BilalQamar95 mentioned this pull request May 22, 2024

feat: updated jest to v29 #1202

Closed

3 tasks

adamstankiewicz approved these changes May 22, 2024

View reviewed changes

BilalQamar95 added 2 commits May 22, 2024 17:57

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

d371695

…n-portal into bilalqamar95/replace-edx/paragon-frontend-build

refactor: pinned paragon version

127bb44

BilalQamar95 merged commit 5ffab67 into master May 22, 2024
6 checks passed

BilalQamar95 deleted the bilalqamar95/replace-edx/paragon-frontend-build branch May 22, 2024 13:17

feanil mentioned this pull request Jun 25, 2024

Jest 26 to 29 Upgrade openedx/public-engineering#239

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace #1192

feat: replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace #1192

BilalQamar95 commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

adamstankiewicz Mar 29, 2024

adamstankiewicz Mar 29, 2024

abdullahwaheed Apr 1, 2024

adamstankiewicz May 20, 2024

adamstankiewicz Mar 29, 2024

adamstankiewicz Mar 29, 2024

BilalQamar95 Apr 1, 2024

adamstankiewicz May 20, 2024 •

edited

Loading

adamstankiewicz May 20, 2024

BilalQamar95 May 21, 2024

adamstankiewicz May 21, 2024

BilalQamar95 May 22, 2024

adamstankiewicz May 20, 2024

adamstankiewicz left a comment

adamstankiewicz May 22, 2024

BilalQamar95 commented May 22, 2024 •

edited

Loading

feat: replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace #1192

feat: replaced packages @edx/paragon & @edx/frontend-build to use openedx namespace #1192

Conversation

BilalQamar95 commented Mar 29, 2024 • edited Loading

codecov bot commented Mar 29, 2024 • 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

adamstankiewicz May 20, 2024 • edited Loading

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

BilalQamar95 commented May 22, 2024 • edited Loading

BilalQamar95 commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

adamstankiewicz May 20, 2024 •

edited

Loading

BilalQamar95 commented May 22, 2024 •

edited

Loading