-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
feat: upgraded to node v18, added .nvmrc and updated workflows #176
Conversation
Codecov ReportPatch and project coverage have no change.
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
☔ View full report in Codecov by Sentry. |
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.
ed7ba42
to
7def9e8
Compare
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.
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.
2c05dce
to
cd8d1d2
Compare
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.
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' }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
There was a problem hiding this comment.
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
cb8bf18
to
8c7e7f8
Compare
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 |
183b3fb
to
039fa87
Compare
.nvmrc
Outdated
@@ -0,0 +1 @@ | |||
18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
18 | |
18.15 |
039fa87
to
b10b450
Compare
b10b450
to
70c3b7b
Compare
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.