Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: upgraded to node v18, added .nvmrc and updated workflows #176

Merged
merged 1 commit into from
May 22, 2023
Merged

feat: upgraded to node v18, added .nvmrc and updated workflows #176

merged 1 commit into from
May 22, 2023

Conversation

Mashal-m
Copy link
Contributor

@Mashal-m Mashal-m commented Mar 31, 2023 •
edited
Loading

  • Added support for node v18, updated .nvmrc and workflows.
  • Updated frontend-platform, frontend-build, frontend-component-header, paragon, and frontend-component-footer versions.

Ticket
Upgrade Node JS from 16 to 18

edx-internal PR

https://github.com/edx/edx-internal/pull/8329

This needs to be merged together.

@codecov
Copy link

codecov bot commented Mar 31, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (7012fa8) 100.00% compared to head (b10b450) 100.00%.

❗ Current head b10b450 differs from pull request most recent head 70c3b7b. Consider uploading reports for the commit 70c3b7b to get more accurate results

Additional details and impacted files 
@@            Coverage Diff            @@
##            master      #176   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          111       111           
  Lines         1080      1080           
  Branches       158       158           
=========================================
  Hits          1080      1080
Impacted Files Coverage Δ
src/index.jsx 100.00% <ø> (ø)

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

@Mashal-m Mashal-m marked this pull request as ready for review April 5, 2023 11:53
@Mashal-m Mashal-m marked this pull request as draft April 20, 2023 11:43
@OmarIthawi OmarIthawi mentioned this pull request Apr 25, 2023
Merged
3 tasks
OmarIthawi added a commit to Zeit-Labs/frontend-app-ora-grading that referenced this pull request Apr 25, 2023
@OmarIthawi
feat: use atlas in make pull_translations
4e11e57 
Changes
-------
 - Bump frontend-platform to bring `intl-imports.js` script
 - Move all i18n imports into `src/i18n/index.js` so `intl-imports.js` can
   override it with latest translations
 - Add `atlas` into `make pull_translations` when `OPENEDX_ATLAS_PULL`
   environment variable is set.
 - package.json and package-lock.json are copied from openedx#176

Refs: [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) implementing Translation Infrastructure OEP-58.
OmarIthawi added a commit to Zeit-Labs/frontend-app-ora-grading that referenced this pull request Apr 25, 2023
@OmarIthawi
feat: use atlas in make pull_translations
0de98c0 
Changes
-------
 - Bump frontend-platform to bring `intl-imports.js` script
 - Move all i18n imports into `src/i18n/index.js` so `intl-imports.js` can
   override it with latest translations
 - Add `atlas` into `make pull_translations` when `OPENEDX_ATLAS_PULL`
   environment variable is set.
 - package.json and package-lock.json are copied from openedx#176
 - updated snapshots and updated tests in sync with `frontend-platform`

Refs: [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) implementing Translation Infrastructure OEP-58.
OmarIthawi added a commit to Zeit-Labs/frontend-app-ora-grading that referenced this pull request Apr 25, 2023
@OmarIthawi
feat: use atlas in make pull_translations
7de0c21 
Changes
-------
 - Bump frontend-platform to bring `intl-imports.js` script
 - Move all i18n imports into `src/i18n/index.js` so `intl-imports.js` can
   override it with latest translations
 - Add `atlas` into `make pull_translations` when `OPENEDX_ATLAS_PULL`
   environment variable is set.
 - package.json and package-lock.json are copied from openedx#176
 - updated snapshots and updated tests in sync with `frontend-platform`
 - require package-lock.json version 3: same as openedx#176

Refs: [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) implementing Translation Infrastructure OEP-58.
brian-smith-tcril pushed a commit that referenced this pull request May 9, 2023
@OmarIthawi
feat: use atlas in make pull_translations (#179)
4158231 
Changes
-------
 - Bump frontend-platform to bring `intl-imports.js` script
 - Move all i18n imports into `src/i18n/index.js` so `intl-imports.js` can
   override it with latest translations
 - Add `atlas` into `make pull_translations` when `OPENEDX_ATLAS_PULL`
   environment variable is set.
 - package.json and package-lock.json are copied from #176
 - updated snapshots and updated tests in sync with `frontend-platform`
 - require package-lock.json version 3: same as #176

Refs: [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) implementing Translation Infrastructure OEP-58.
@Mashal-m Mashal-m marked this pull request as ready for review May 10, 2023 12:55
leangseu-edx
leangseu-edx reviewed May 12, 2023
View reviewed changes
src/data/constants/app.test.js Outdated
@@ -17,8 +17,8 @@ describe('app constants', () => {
});
test('locationId returns trimmed pathname', () => {
const old = window.location;
window.location = { pathName: '/somePath.jpg' };
window.location ??= { pathName: '/somePath.jpg' };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed the intention of the previous code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to add this to fix failing tests.

Copy link
Contributor

@leangseu-edx leangseu-edx May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this is a bug on node 18. nodejs/node#47563 . I don't like the solution to force this code in right now then remember to remove it later. I would rather a wait until the next release of node 18.x and not change this test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Contributor Author

@Mashal-m Mashal-m May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pinned the node version to 18.15 until the next release.
@leangseu-edx

@leangseu-edx
Copy link
Contributor

One requested change. I will approve once this get update.

@abdullahwaheed
Copy link
Contributor

One requested change. I will approve once this get update.

@leangseu-edx we have pinned down node version and updated code according to your suggestion. could you please review and merge this

@leangseu-edx
Copy link
Contributor

One requested change. I will approve once this get update.

@leangseu-edx we have pinned down node version and updated code according to your suggestion. could you please review and merge this

Should we include that version .nvmrc as well?

leangseu-edx
leangseu-edx approved these changes May 22, 2023
View reviewed changes
.nvmrc Outdated
@@ -0,0 +1 @@
18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
18
18.15

@leangseu-edx leangseu-edx merged commit 33ba1cd into master May 22, 2023
@leangseu-edx leangseu-edx deleted the mashal-m/node-18-upgrade branch May 22, 2023 15:20
@ghassanmas ghassanmas mentioned this pull request Jun 6, 2023
Closed
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muselesscreator muselesscreator muselesscreator left review comments

@leangseu-edx leangseu-edx leangseu-edx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mashal-m @leangseu-edx @abdullahwaheed @muselesscreator