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.

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

Migrate cwa-website to use node16 #2282

Merged
merged 3 commits into from
Feb 4, 2022
Merged

Migrate cwa-website to use node16 #2282

merged 3 commits into from
Feb 4, 2022

Conversation

MikeMcC399
Copy link
Contributor

@MikeMcC399 MikeMcC399 commented Jan 5, 2022

This PR follows on from issue #2155 "Website migration steps for Nodejs 16" and implements the steps required to migrate this repository to use Nodejs version 16.

The Nodejs version selected is lts/gallium which is equivalent to the cached version 16.13.2 specified in the Ubuntu 20.04.3 LTS - Readme

I have tested it locally on Ubuntu 20.04 and Windows 11 with the Cypress test suite.

I also tested the workflows on a private clone of the repository. The only portion I cannot test is the final deployment in deploy-master.yml using jakejarvis/s3-sync-action@master.


Internal Tracking ID: EXPOSUREAPP-10928

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Jan 5, 2022

Hi Mike, thanks for this PR! I have some questions:

Could you elaborate on the consequences for contributors? Do I need to update to node16 manually when this change is merged? Anything else I should note with node16?

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

Thank you for your questions.

If you continue to use Nodejs 14 you will get a warning if you run npm install:

npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it!

You can still use the commands to build and run the web locally,

npm run build
npm start

For anybody who is contributing only text inputs to the repository, they could continue to use Nodejs 14 in the interim, however it would be recommended to update to Nodejs 16 at some time.

Anyone who is updating package versions or doing more technical work should probably update to Nodejs 16 before making any significant contributions.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Jan 5, 2022

Okay, thanks for the explanations @MikeMcC399!

@cwa-bot cwa-bot bot moved this from ToDo to Mirrored to Jira in [CM] cwa-website Jan 5, 2022
@MikeMcC399 MikeMcC399 marked this pull request as ready for review January 7, 2022 10:23
@MikeMcC399 MikeMcC399 requested a review from a team January 7, 2022 10:23
@MikeMcC399

This comment has been minimized.

@MikeMcC399
Copy link
Contributor Author

In the meantime Node lts/gallium equates to Node 16.13.2 (previously 16.13.1) and this is cached in GitHub's virtual environment Ubuntu 20.04.3 LTS.

Also there are some new minor versions of some npm packages.

If there is interest in pursuing this PR I will update and test it with the newer patches and minor versions, otherwise I leave it as it is.

@dsarkar
Copy link
Member

dsarkar commented Jan 17, 2022

@MikeMcC399 Indeed, there is interest in pursuing this PR! Thanks!

@MikeMcC399
Copy link
Contributor Author

@dsarkar

Indeed, there is interest in pursuing this PR!

That is good to hear! I will update the PR soon in that case (rebase / re-install / re-test).

@MikeMcC399
Copy link
Contributor Author

@dsarkar
This PR is updated. It still passes the Cypress test suite with
$ npm test
on Ubuntu 20.04.

@MikeMcC399
Copy link
Contributor Author

Rebased once again to resolve conflicts with master branch.

@svengabr
Copy link
Member

Oh damn @MikeMcC399 I just did a manual merge conflict as well, currently looking into getting it running with node16

@MikeMcC399
Copy link
Contributor Author

@svengabr

Oh damn @MikeMcC399 I just did a manual merge conflict as well, currently looking into getting it running with node16

Do you want me to force push again?

Sorry that we were working in parallel!

@MikeMcC399
Copy link
Contributor Author

@svengabr

Let me check again that all is working on node 16, then I will update the branch once more.

@MikeMcC399 MikeMcC399 marked this pull request as draft January 26, 2022 13:26
@svengabr
Copy link
Member

@MikeMcC399 you can continue what you were working on. I wanted to fix the merge conflict and then test it using

nvm install 16.13.2
nvm use 16.13.2
npm ci
npm run build

After that, I wanted to have it deployed to the non-productive test environment for a quick review

@MikeMcC399
Copy link
Contributor Author

@svengabr
Stand by. I will post (hopefully) positive results from cypress test.

@MikeMcC399
Copy link
Contributor Author

@svengabr

I wanted to fix the merge conflict and then test it using

nvm install 16.13.2
nvm use 16.13.2
npm ci
npm run build

After that, I wanted to have it deployed to the non-productive test environment for a quick review

It would be good to run those tests manually, however in fact they are already checked automatically (see https://github.com/corona-warn-app/cwa-website/pull/2282/checks).

To your test suite, I suggest to add:

npm run test:prepare
npm test

to run the Cypress test suite.

I just did that, with the following result:

    Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  applink_spec.js                          00:02        3        3        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  blog_spec.js                             00:03        2        2        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  check_anchor_links.js                    01:26       31       31        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  check_links.js                           15:07       31       31        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  hotline_spec.js                          915ms        1        1        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  pages/eventRegistration.js               00:10        6        6        -        -        - │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        16:51       74       74        -        -        -  

$ node -v
v16.13.2

@MikeMcC399 MikeMcC399 marked this pull request as ready for review January 26, 2022 13:53
@MikeMcC399
Copy link
Contributor Author

@svengabr
Ready for you again. 🙂

@svengabr
Copy link
Member

@MikeMcC399 looks all good to me. Is anything missing from your side/any open tasks or can we consider merging into main?

@MikeMcC399
Copy link
Contributor Author

MikeMcC399 commented Jan 26, 2022

@svengabr

Is anything missing from your side/any open tasks or can we consider merging into main?

There are no open tasks on my side, so you are welcome to go ahead and merge into master.

(There is no main branch (yet) in this repository. The default branch is master.)

@MikeMcC399
Copy link
Contributor Author

Rebased and retested successfully on Ubuntu 20.04 and Windows 11

nvm use 16.13.2
npm ci
npm run test:prepare
npm test

√ All specs passed!

@svengabr svengabr marked this pull request as ready for review February 3, 2022 08:06
@MikeMcC399
Copy link
Contributor Author

@svengabr / @dsarkar

I did update this yesterday. Does this mean that you are ready to go with it?

@svengabr
Copy link
Member

svengabr commented Feb 3, 2022

@MikeMcC399
Yes, we are ready to go with it. Golive is planned for tomorrow morning.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 3, 2022

@MikeMcC399 FYI: https://covidapps.slack.com/archives/C0194ML0MLN/p1643874479792409

@MikeMcC399
Copy link
Contributor Author

@svengabr

Yes, we are ready to go with it. Golive is planned for tomorrow morning.

Thank you for letting me know. I will not touch the PR unless you ask me to. I have retested the current contents on Ubuntu and Windows 11 successfully.

@Ein-Tim
Thank you for the Slack link. I stopped following Slack because at the time there was very little active involvement from the Open Source Team. If that has now changed I will start following again.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 3, 2022

@svengabr I can offer to test this on my Mac Book Air running the latest macOS version.

@MikeMcC399
For sure! This was the first message in a long time from @svengabr on Slack, so I don't know whether there will be more frequent posts from him again.

If I test this PR, I'd need to install node16, right?

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

If I test this PR, I'd need to install node16, right?

Yes, you need node 16 to test with node 16 😆

Conversation continued in Slack....

Copy link
Contributor

@Ein-Tim Ein-Tim left a comment

Choose a reason for hiding this comment

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

This change breaks building the page locally on my M1 MacBook.

The terminal says:
Error: Node Sass does not yet support your current environment: OS X Unsupported architecture (arm64) with Node.js 16.x

In sass/node-sass#3033 you can read that Node Saas does not yet support Apple ARM.

However, it seems like I can continue using Node.js version 14.18.3 without having any difficulties to build the page. I'm not even seeing a warning message like the one @MikeMcC399 posted above.

So from my POV, this is not an optimal change, however, I also won't block it.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 3, 2022

I just started to wonder: Aren't we already using Node Saas currently?

If yes, I wonder why I only see this message when testing this PR and not when building the master branch. 🤔

Edit: The error message actually speaks from Node.js 16, so maybe Node Saas was updated, etc.?

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

Your error message is part of the problem that Node Sass is deprecated (see https://www.npmjs.com/package/node-sass).

"node-sass

Warning: LibSass and Node Sass are deprecated. While they will continue to receive maintenance releases indefinitely, there are no plans to add additional features or compatibility with any new CSS or Sass features. Projects that still use it should move onto Dart Sass."

I'm sorry that I didn't realise that macOS users like yourself could be impacted.

It is possible to migrate to Dart Sass but this means some other technical changes to the website. I did try it out a couple of months ago and I would have suggested it at the time if it had been 100% compatible.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 4, 2022

@MikeMcC399

I'm sorry that I didn't realise that macOS users like yourself could be impacted.

No problem. As stated above, this is not optimal for me, but I will just continue to use Node.js 14.18.3. I hope this works!

@svengabr
Copy link
Member

svengabr commented Feb 4, 2022

Delaying a little bit (later today) until I have internal results using a M1 macbook

@svengabr
Copy link
Member

svengabr commented Feb 4, 2022

A colleague just tested using an M1 Macbook with NodeJS 16 and everything worked as intended with no errors. From my perspective, we are good to go.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 4, 2022

I wonder what caused this error then.
Feel free to merge already, I will retest later with a clean fork of cwa-website and a new install of nvm.

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

I will retest later with a clean fork of cwa-website and a new install of nvm.

That would be helpful. I suggest to open a new issue to look at this after this PR is merged.

@svengabr svengabr linked an issue Feb 4, 2022 that may be closed by this pull request
@svengabr svengabr merged commit 9e5d6d9 into corona-warn-app:master Feb 4, 2022
@cwa-bot cwa-bot bot moved this from Mirrored to Jira to Done in [CM] cwa-website Feb 4, 2022
@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

I suggest to begin with just do:

git switch master
git fetch upstream
git merge upstream/master
npm ci
npm run build

without upgrading to nvm 16.

That seems to be no problem on Windows at least.

@MikeMcC399 MikeMcC399 deleted the migrate-node16 branch February 4, 2022 10:11
@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 4, 2022

@MikeMcC399 I'll try this now.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Feb 4, 2022

FYI: I will follow up in #2411.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Website migration steps for Nodejs 16
4 participants