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 #977

Merged
merged 33 commits into from
Jul 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
6aa6fed
feat: upgraded to node v18, added .nvmrc and updated workflows
BilalQamar95 Mar 15, 2023
372e6dd
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 Apr 4, 2023
6203d67
Merge branch 'master' into bilalqamar95/node-v18-upgrade
BilalQamar95 Apr 4, 2023
a66b322
Merge branch 'bilalqamar95/node-v18-upgrade' of https://github.com/op…
BilalQamar95 May 11, 2023
12fb6a7
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 May 11, 2023
80b867d
refactor: updated packages
BilalQamar95 May 11, 2023
c8c1537
refactor: updated packages
BilalQamar95 May 15, 2023
15eb05d
refactor: updated packages
BilalQamar95 May 15, 2023
ebc18a7
refactor: updated frontend-build
BilalQamar95 May 15, 2023
7f7be04
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 May 16, 2023
ae8397e
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 May 31, 2023
91a9495
refactor: updated packages
BilalQamar95 May 31, 2023
74df5e1
refactor: updated packages
BilalQamar95 Jun 1, 2023
85898a9
refactor: updated packages and resolved typescript issue
BilalQamar95 Jun 1, 2023
482b527
refactor: updated enterprise, platform packages
BilalQamar95 Jun 2, 2023
b3972c8
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 Jun 2, 2023
9745b31
refactor: updated packages and snapshots
BilalQamar95 Jun 2, 2023
1c60555
refactor: updated packages & lockfileversion workflow
BilalQamar95 Jun 2, 2023
1c63db1
refactor: updated postcss
BilalQamar95 Jun 2, 2023
2a96d0b
refactor: pinned node version to 18.15
BilalQamar95 Jun 2, 2023
ca478ef
refactor: reverted frontend-build version & updated packages
BilalQamar95 Jun 5, 2023
59a6a50
refactor: updated frontend-build
BilalQamar95 Jun 5, 2023
2f9920f
refactor: updated frontend-build to v12.9.0.alpha.1
BilalQamar95 Jun 19, 2023
5a40e60
Merge branch 'master' of https://github.com/openedx/frontend-app-admi…
BilalQamar95 Jun 19, 2023
45f4605
refactor: updated packages and snapshots
BilalQamar95 Jun 20, 2023
f55730b
refactor: updated testing-library dom & jest-dom packages
BilalQamar95 Jun 20, 2023
4541b8b
refactor: updated packages and snapshots
BilalQamar95 Jun 21, 2023
d450b09
Merge branch 'master' into bilalqamar95/node-v18-upgrade
BilalQamar95 Jun 21, 2023
d4b7325
Merge branch 'master' into bilalqamar95/node-v18-upgrade
BilalQamar95 Jul 4, 2023
8250251
refactor: updated nvmrc & removed babel.config
BilalQamar95 Jul 7, 2023
e627265
Merge branch 'master' into bilalqamar95/node-v18-upgrade
BilalQamar95 Jul 12, 2023
6a6c2f3
Merge branch 'master' into bilalqamar95/node-v18-upgrade
adamstankiewicz Jul 13, 2023
e05fd4f
Merge branch 'master' into bilalqamar95/node-v18-upgrade
BilalQamar95 Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,15 @@ on:
jobs:
tests:
runs-on: ubuntu-latest
strategy:
matrix:
node: [16]
npm: [8.5.x]
steps:
- name: Checkout
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Setup Nodejs Env
run: echo "NODE_VER=`cat .nvmrc`" >> $GITHUB_ENV
- name: Setup Nodejs
uses: actions/setup-node@v1
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}
- run: npm install -g npm@${{ matrix.npm }}
node-version: ${{ env.NODE_VER }}
Copy link
Member

Choose a reason for hiding this comment

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

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}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

- name: Validate no uncommitted package-lock changes
run: make validate-no-uncommitted-package-lock-changes
- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lockfileversion-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ on:

jobs:
version-check:
uses: edx/.github/.github/workflows/lockfileversion-check.yml@master
uses: openedx/.github/.github/workflows/lockfileversion-check-v3.yml@master
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
16
18.15
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

2 changes: 2 additions & 0 deletions babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@ const { getBaseConfig } = require('@edx/frontend-build');

const config = getBaseConfig('babel');

config.presets.push('@babel/preset-typescript');
Copy link
Member

Choose a reason for hiding this comment

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

[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.

Copy link
Contributor Author

@BilalQamar95 BilalQamar95 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


module.exports = config;
Copy link
Member

Choose a reason for hiding this comment

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

[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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Loading