search

CATcher-org / CATcher

CATcher is a software application used for peer-testing of software projects.
https://catcher-org.github.io/CATcher/
MIT License
71 stars 66 forks source link

Upgrade to Angular 13 #1249

Closed cheehongw closed 2 months ago

cheehongw commented 4 months ago

Summary:

Upgrade to Angular 13 and Node 16 This pull request addresses some of the outdated packages in our application.

Partially addresses #1192

Changes Made:

Some of our packages are old and outdated. We should actively maintain and keep these packages up-to-date so it is easier to maintain in the future.

Let's upgrade to Angular 13 to keep our packages up-to-date.

cheehongw commented 4 months ago

Loading service still works

https://github.com/CATcher-org/CATcher/assets/72195240/0988826f-38a3-469f-8384-54c768ea1bad

luminousleek commented 2 months ago

System

OS: macOS Sonoma 14.4.1 Browser: Safari Browser Version: Version 17.4.1 (19618.1.15.11.14) Local Angular Version: 12.2.18 node Version: v14.19.3 npm Version: 6.14.17

Actions Run

npm install: success npm run ng:serve:web: - had to run npx browserslist --updatedb for it to work npm run test: - had to delete package-lock.json and node-modules and re-run npm install for it to work npm run e2e: success

Tested Phases

  1. Bug Reporting Phase
  2. Team's Response Phase
  3. Tester's Response Phase

No bugs found

luminousleek commented 2 months ago

Once we use Node 16 for the GitHub CI tests we need to update the branch protection to require linux-setup-and-tests (16.x) instead of linux-setup-and-tests (14.x)

cheehongw commented 2 months ago

Finally got the CI working. This comment will detail the changes made to the environment for the Angular 13 upgrade:

cheehongw commented 2 months ago

System

OS: Windows 10 Browser: Chrome Browser Version: 124.0.6367.91 Local Angular Version: 13.4.0 node Version: 16.20.2 npm Version: 8.19.4

Actions Run

npm install npm run ng:serve:web npm run test npm run e2e

All success

Tested Phases

  1. Bug Reporting Phase
  2. Team's Response Phase
  3. Tester's Response Phase

No bugs found

chunweii commented 2 months ago

Finally got the CI working. This comment will detail the changes made to the environment for the Angular 13 upgrade:

  • Upgrade to Node 16, npm8 from Node14, npm6

  • @octokit/core isn't used directly as a dependency in our project, but @octokit/rest depends on it and npm8 installs the latest one (v6.1.2), which doesnt support Node 16.

    • As such, this PR includes a new dev dependency to freeze @octokit/core at v4, which is the last version to support Node 16
    • We should consider removing it once we upgrade to Angular 15, which supports Node 18.
  • Manually install firefox for the macos-latest runner. As of this comment, macos-latest uses a macos 14 arm64 image, which doesnt include firefox. This seems to be a very recent change, since macos-latest always pointed to a x64 image which came pre-installed with firefox.

Thanks for the fix @cheehongw! I have some comments on the octokit dependency.

  1. Dev dependencies should be reserved for dependencies that are only used during development and not needed in production. CATcher and WATcher has been so far installing everything for use in production so putting @octokit/core into devDependencies should not break stuff, but I think the best practice is to put it into the dependencies instead.
  2. Also, a better way to resolve the octokit issue would be to upgrade @octokit/rest to ^18.12.0, but this can cause some breaking changes (we need to change some parts of our code).
cheehongw commented 2 months ago

Thanks for the review @chunweii

  1. Ok, will move octokit/core to dependencies
  2. Any reason for targeting octokit/rest@18.12.0? I am unable to find anything in the octokit/rest release notes about changing how octokit/core is depended upon.
    • Either way and as you mentioned, the task of upgrading octokit/rest has alot of breaking changes. Going from v16 -> v17 alone seems to introduce quite a bit of breaking changes. As such, I think this is better off as a separate PR since it is not related to Angular 13.
cheehongw commented 2 months ago

System

OS: Ubuntu 22.04 Browser: Chrome Browser Version: 123.0.6312.122 Local Angular Version: 13.4.0 node Version: 16.20.2 npm Version: 8.19.4

Actions Run

npm install npm run ng:serve:web npm run test npm run e2e

All success

Tested Phases

  1. Bug Reporting Phase
  2. Team's Response Phase
  3. Tester's Response Phase

No bugs found

chunweii commented 2 months ago

  1. Any reason for targeting octokit/rest@18.12.0? I am unable to find anything in the octokit/rest release notes about changing how octokit/core is depended upon.

    • Either way and as you mentioned, the task of upgrading octokit/rest has alot of breaking changes. Going from v16 -> v17 alone seems to introduce quite a bit of breaking changes. As such, I think this is better off as a separate PR since it is not related to Angular 13.

This old version of octokit (v16) does not support the use of ESM imports (I don't see it in the documentations), which imo makes the type checking better.

Furthermore, one of the dependencies in octokit/rest is causing the problem of installing an incompatible version of octokit/core. I tried installing v18 and there were no issues with installation afterwards (but I could also have tried 17, or some other version)

But I agree we should not upgrade octokit in this PR