Updated frontend-build to v12 #270

BilalQamar95 · 2022-08-12T08:26:14Z

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 post update

Merge checklist:

Evaluate how your changes will impact existing consumers (e.g., frontend-app-learner-portal-enterprise, frontend-app-admin-portal, and frontend-app-enterprise-public-catalog). Will consumers safely be able to upgrade to this change without any breaking changes?
Ensure your commit message follows the semantic-release conventional commit message format. If your changes include a breaking change, ensure your commit message is explicitly marked as a BREAKING CHANGE so the NPM package is released as such.
Once CI is passing, verify the package versions that Lerna will increment to in the Github Action CI workflow logs.
- Note: This may be found in the "Preview Updated Versions (dry run)" step in the Github Action CI workflow logs.

Post merge:

Verify Lerna created a release commit (e.g., chore(release): publish) that incremented versions in relevant package.json and CHANGELOG files, and created Git tags for those versions.
Run the Publish from package.json Github Action workflow to publish these new package versions to NPM.
- This may be triggered by clicking the "Run workflow" option for the master branch.
Verify the new package versions were published to NPM (i.e., npm view <package_name> versions --json).
- Note: There may be a slight delay between when the workflow finished and when NPM reports the package version as being published. If it doesn't appear right away in the above command, try again in a few minutes.

…-search

codecov · 2022-08-12T08:30:47Z

Codecov Report

Merging #270 (7c1d304) into master (4776949) will increase coverage by 0.11%.
The diff coverage is 75.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   77.46%   77.58%   +0.11%     
==========================================
  Files          34       34              
  Lines         648      647       -1     
  Branches      162      162              
==========================================
  Hits          502      502              
+ Misses        133      132       -1     
  Partials       13       13

Impacted Files	Coverage Δ
...ckages/catalog-search/src/SearchSuggestionItem.jsx	`100.00% <ø> (ø)`
packages/catalog-search/src/SearchSuggestions.jsx	`100.00% <ø> (ø)`
packages/utils/src/test-utils.jsx	`0.00% <0.00%> (ø)`
packages/catalog-search/src/data/reducer.js	`94.28% <100.00%> (ø)`
packages/logistration/src/LoginRedirect.jsx	`100.00% <100.00%> (ø)`
packages/utils/src/analytics.js	`88.88% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4776949...7c1d304. Read the comment docs.

long74100 · 2022-08-26T15:21:39Z

packages/catalog-search/package.json

@@ -44,7 +44,7 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.1.0",
-    "@edx/frontend-build": "11.0.1",
+    "@edx/frontend-build": "^12.0.3",


Should we pin the version?

long74100 · 2022-08-26T15:28:31Z

packages/catalog-search/src/SearchSuggestions.jsx


 SearchSuggestions.propTypes = {
-  autoCompleteHits: PropTypes.arrayOf(PropTypes.object).isRequired,
+  autoCompleteHits: PropTypes.arrayOf(PropTypes.shape({})).isRequired,


nit: maybe we can include content_type here?

adamstankiewicz · 2022-09-07T16:41:22Z

packages/catalog-search/package.json

@@ -44,7 +44,7 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.1.0",
-    "@edx/frontend-build": "11.0.1",
+    "@edx/frontend-build": "^12.0.3",


nit: keep @edx/frontend-build pinned like the other dependencies.

adamstankiewicz · 2022-09-07T16:42:53Z

packages/catalog-search/src/SearchSuggestionItem.jsx

@@ -45,7 +45,7 @@ SearchSuggestionItem.propTypes = {
    key: PropTypes.string,
    title: PropTypes.string,
    program_type: PropTypes.string,
-    authoring_organizations: PropTypes.array,
+    authoring_organizations: PropTypes.shape([]),


PropTypes.shape is usually reserved for an object prop, not an array. Should this be PropTypes.arrayOf(PropTypes.shape()) instead?

Related, is this hit.authoring_organizations actually used in this component? If not, is it necessary to include in the prop types definition?

authoring_organizations is not used in this component. Removed it from prop types definition

adamstankiewicz · 2022-09-07T16:44:21Z

packages/catalog-search/src/SearchSuggestions.jsx


 SearchSuggestions.propTypes = {
-  autoCompleteHits: PropTypes.arrayOf(PropTypes.object).isRequired,
+  autoCompleteHits: PropTypes.arrayOf(PropTypes.shape({})).isRequired,


adamstankiewicz · 2022-09-07T16:49:45Z

packages/catalog-search/src/tests/SearchPagination.test.jsx

+    const { history } = renderWithRouter(<SearchData>
+      <SearchPaginationBase nbPages={4} currentRefinement={3} />
+    </SearchData>, {
+      route: 'search/?page=3',
+    });


This formatting looks a bit funky. Am I correct in thinking it also causes the ESLint error that you disabled on line 1 (react/jsx-closing-tag-location)?

Would something like the following work?

const { history } = renderWithRouter(( <SearchData> <SearchPaginationBase nbPages={4} currentRefinement={3} /> </SearchData>, ), { route: 'search/?page=3', });

adamstankiewicz · 2022-09-07T16:50:09Z

packages/hotjar/package.json

@@ -38,7 +38,7 @@
  "sideEffects": false,
  "devDependencies": {
    "@edx/browserslist-config": "1.1.0",
-    "@edx/frontend-build": "11.0.1",
+    "@edx/frontend-build": "^12.0.3",


nit: pin this dependency like all the other dependencies are pinned.

adamstankiewicz · 2022-09-07T16:50:16Z

packages/logistration/package.json

@@ -42,7 +42,7 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.1.0",
-    "@edx/frontend-build": "11.0.1",
+    "@edx/frontend-build": "^12.0.3",


nit: pin this dependency like all the other dependencies are pinned.

adamstankiewicz · 2022-09-07T16:51:00Z

packages/utils/package.json

@@ -42,7 +42,7 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.1.0",
-    "@edx/frontend-build": "11.0.1",
+    "@edx/frontend-build": "^12.0.3",


nit: pin this dependency like all the other dependencies are pinned.

adamstankiewicz · 2022-09-07T16:51:55Z

packages/catalog-search/src/tests/SearchFilters.test.jsx

+      // eslint-disable-next-line react/jsx-no-constructed-context-values
      <ResponsiveContext.Provider value={{ width: breakpoints.large.maxWidth }}>


Other ESLint PRs I looked at recently resolved this error in most tests versus ignoring the ESLint error. Why are we opting to ignore the error here versus fixing it?

adamstankiewicz · 2022-09-07T16:54:25Z

packages/logistration/src/LoginRedirect.test.jsx

+// eslint-disable-next-line import/no-unresolved
 import '@testing-library/jest-dom/extend-expect';


[curious] why did this import need to disable the import/no-unresolved ESLint rule when import '@testing-library/jest-dom/extend-expect'; is used in several other files, seemingly without the ESlint error? Why do we only have to ignore the error here? Or, conversely, can we avoid needing to disable this rule since it doesn't seem relevant for the other places where we're importing this same package in other test files.

… into bilalqamar95/frontend-build-upgrade

…-search

…ation

adamstankiewicz

LGTM. @BilalQamar95 are you familiar with the release process in this repo? It does require a manual step to trigger a Github Action workflow.

Also, when you merge, ensure the merged commit will follow the semantic-release commit format. I'd also consider whether these changes are technically a breaking change or not?

arbrandes · 2023-01-23T18:33:47Z

@BilalQamar95, any hitches here?

BilalQamar95 · 2023-01-24T11:31:50Z

Seems good to go, also I don't think these changes are technically breaking change

BilalQamar95 · 2023-01-24T13:19:04Z

LGTM. @BilalQamar95 are you familiar with the release process in this repo? It does require a manual step to trigger a Github Action workflow.

Also, when you merge, ensure the merged commit will follow the semantic-release commit format. I'd also consider whether these changes are technically a breaking change or not?

Hi @adamstankiewicz, so I did follow the release process & manually triggered the Publish from package.json Github Action workflow found here. The release workflow is still failing on Increment versions of Changed packages. I was wondering what am I missing here.

adamstankiewicz · 2023-01-25T13:02:55Z

@BilalQamar95 Huh, interesting... the Release workflow failed yet you still managed to get it published to NPM (e.g., @edx/frontend-enterprise-catalog-search is at the correct 3.1.6 version on NPM seemingly your changes included as well as the appropriate CHANGELOG entries, etc.) 🤯

I believe what happened is that the first run of the Release workflow when you merged failed to this relatively new change to the master branch requirements from tCRIL:

lerna WARN gitPush remote: error: GH006: Protected branch update failed for refs/heads/master.        
lerna WARN gitPush remote: error: Required status check "openedx/cla" is expected.

Any commit to master must now go have a signed, passing openedx/cla agreement like the CI check for PR authors, and this is the first time we've tried releasing any of these packages since that new requirement.

The result from a successful run of the Release workflow is Lerna committing directly to master via the semantic-release user, which does not pass the openedx/cla agreement. As such, Lerna can't commit the required publish commit to master.

However, it does look like it was able to push the tags for the new versions. Because of this, even though there was no publish commit to master when you manually triggered the "Publish from package.json" Github Action workflow, the tags existed so semantic-release knew to publish new versions, and must have recognized the CHANGELOG and package.json version increment wasn't included so it added it pre-release in the workflow itself (which does not attempt commit to master).

The situation we're in now is that we're missing the package.json, package-lock.json, and CHANGELOG.md updates for all the changed packages from this PR on master that are actually published on NPM.

For example, you can see your changes in the NPM code explorer on the version published yesterday but, if you look in the CHANGELOG on master (link), v3.1.6 is not present even though it is on NPM.

We need a way to get those changes back into master, either manually or by re-running semantic-release in some manner to generate it. I think for the short term we should disable the openedx/cla check until this repo can migrate to an alternative approach that doesn't fail the openedx/cla check (e.g., rather than committing directly to master, could we get the Lerna output into a PR with the Github bot and automerged).

BilalQamar95 · 2023-01-25T13:24:45Z

@adamstankiewicz Thank you for such detailed explanation.

BilalQamar95 added 2 commits August 12, 2022 12:10

refactor: updated frontend-build & resolved eslint issues for catalog…

303429f

…-search

refactor: updated frontend-build to v12 & resolved eslint issues

1bdbdaf

BilalQamar95 self-assigned this Aug 12, 2022

abdullahwaheed added the engineering review label Aug 25, 2022

long74100 reviewed Aug 26, 2022

View reviewed changes

adamstankiewicz reviewed Sep 7, 2022

View reviewed changes

BilalQamar95 added 9 commits September 19, 2022 13:47

refactor: pinned frontend-build version & eslint update

8828608

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

2e880dc

… into bilalqamar95/frontend-build-upgrade

refactor: resolved eslint issues post master branch merge

2550b1d

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

0b05d2a

… into bilalqamar95/frontend-build-upgrade

refactor: updated frontend-build & resolved eslint issues for catalog…

ee83ad8

…-search

refactor: updated frontend-build & resolved eslint issues for hotjar

eb3c507

refactor: updated frontend-build & resolved eslint issues for logistr…

85c35cd

…ation

refactor: updated frontend-build & resolved eslint issues for utils

174b230

refactor: updated package-lock

7c1d304

arbrandes mentioned this pull request Nov 1, 2022

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

Closed

27 tasks

adamstankiewicz approved these changes Jan 12, 2023

View reviewed changes

BilalQamar95 merged commit 5f7e948 into master Jan 24, 2023

BilalQamar95 deleted the bilalqamar95/frontend-build-upgrade branch January 24, 2023 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated frontend-build to v12 #270

Updated frontend-build to v12 #270

BilalQamar95 commented Aug 12, 2022

codecov bot commented Aug 12, 2022 •

edited

Loading

long74100 Aug 26, 2022

long74100 Aug 26, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

BilalQamar95 Sep 19, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz Sep 7, 2022

adamstankiewicz left a comment

arbrandes commented Jan 23, 2023

BilalQamar95 commented Jan 24, 2023

BilalQamar95 commented Jan 24, 2023

adamstankiewicz commented Jan 25, 2023

BilalQamar95 commented Jan 25, 2023

		// eslint-disable-next-line react/jsx-no-constructed-context-values
		<ResponsiveContext.Provider value={{ width: breakpoints.large.maxWidth }}>

		// eslint-disable-next-line import/no-unresolved
		import '@testing-library/jest-dom/extend-expect';

Updated frontend-build to v12 #270

Updated frontend-build to v12 #270

Conversation

BilalQamar95 commented Aug 12, 2022

codecov bot commented Aug 12, 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

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

arbrandes commented Jan 23, 2023

BilalQamar95 commented Jan 24, 2023

BilalQamar95 commented Jan 24, 2023

adamstankiewicz commented Jan 25, 2023

BilalQamar95 commented Jan 25, 2023

codecov bot commented Aug 12, 2022 •

edited

Loading