feat: upgraded to node v18, added .nvmrc and updated workflows #977
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #977 +/- ##
=======================================
Coverage 83.18% 83.18%
=======================================
Files 397 397
Lines 8639 8639
Branches 1787 1787
=======================================
Hits 7186 7186
Misses 1415 1415
Partials 38 38
☔ View full report in Codecov by Sentry. |
…n-portal into bilalqamar95/node-v18-upgrade
.nvmrc
Outdated
| @@ -1 +1 @@ | |||
| 16 | |||
| 18.15 | |||
There was a problem hiding this comment.
I would recommend setting this as just the major version 18 (i.e., no specified minor version) so engineers don't necessarily need explicitly Node 18.15 but can rather use any version of Node 18.
There was a problem hiding this comment.
You are correct, but in this particular case I pinned it because of "TypeError: Cannot redefine property: window" issue. It was a bug fixed by nodejs/node#46615 but it wasn't back-ported to v18.16 at the time when I made these commits thus causing the aforementioned error, to resolve the issue I pinned it to v18.15. It seems that is has since been back-ported and the issue is now resolved. I have updated the version in .nvmrc.
babel.config.js
Outdated
| @@ -3,4 +3,6 @@ const { getBaseConfig } = require('@edx/frontend-build'); | |||
|
|
|||
| const config = getBaseConfig('babel'); | |||
|
|
|||
| config.presets.push('@babel/preset-typescript'); | |||
There was a problem hiding this comment.
[clarification] Is this addition necessary given @edx/frontend-build@alpha is installed, which has both the default webpack.dev.config.js and webpack.prod.config.js files using the babel-typescript ConfigPreset (e.g., webpack.prod.config.js)?
See babel-typescript.config.js in @edx/frontend-build (source) for where @babel/preset-typescript is already added.
tl;dr; I believe general TypeScript configs like this one should live in the default configs provided by @edx/frontend-build, not in repo-specific configs.
There was a problem hiding this comment.
Yes, you are right. I ran into ts import errors as it seemed Babel did not understand TypeScript syntax, this addition resolved that issue but while exploring further I came across @edx/frontend-build@alpha. With @edx/frontend-build@alpha installed this is no longer necessary/required.
babel.config.js
Outdated
| @@ -3,4 +3,6 @@ const { getBaseConfig } = require('@edx/frontend-build'); | |||
|
|
|||
| const config = getBaseConfig('babel'); | |||
|
|
|||
| config.presets.push('@babel/preset-typescript'); | |||
|
|
|||
| module.exports = config; | |||
There was a problem hiding this comment.
[suggestion] It looks like babel.config.js in this repo doesn't actually extend/override any of the Babel configuration. I might recommend we simply delete babel.config.js given its relying on the default configuration from @edx/frontend-build anyways. We can always re-add the file in the repo if/when we need to customize the Babel configuration specific to this repo, but as far as I can tell, this should be unnecessary at the moment.
There was a problem hiding this comment.
Thank you for your suggestion, we agree that the current babel.config.js file does not extend/override any of the Babel configuration and it is relying on the default configuration from @edx/frontend-build. Considering this, I have removed babel.config.js in this repo.
| with: | ||
| node-version: ${{ matrix.node }} | ||
| - run: npm install -g npm@${{ matrix.npm }} | ||
| node-version: ${{ env.NODE_VER }} |
There was a problem hiding this comment.
Just curious if the node-version-file attribute was considered when constructing the workflow. We could retain the original strategy matrix attributes, but point directly to the .nvmrc file as a reference to the intended node version to upgrade to.
Docs: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file
strategy:
matrix:
node-version-file: ['.nvmrc']
...
steps:
...
- name: Setup Nodejs Env version ${{ matrix.node-version-file}}
uses: actions/setup-node@v3
with:
node-version-file: ${{matrix.node-version-file}}There was a problem hiding this comment.
The workflow was created by arbi-bom originally and with node v18 upgrades it spread across all the remaining MFEs where it was not being used previously. I'm not sure if node-version-file was considered while constructing it but I could see the merit to retain the original strategy matrix attributes. We can explore this further, thanks for bringing it to our attention.
edx-internal PR
https://github.com/edx/edx-internal/pull/8353
this needs to be merged togther
For all changes
Only if submitting a visual change