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

ci(tests): Update nodejs versions #715

Merged
merged 1 commit into from
May 3, 2022
Merged

Conversation

Falci
Copy link
Member

@Falci Falci commented Apr 19, 2022

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 64.561% when pulling 1fc2bc1 on Falci:node-18 into 35ea46d on handshake-org:master.

@coveralls
Copy link

coveralls commented Apr 19, 2022

Coverage Status

Coverage remained the same at 64.602% when pulling ca8eaa8 on Falci:node-18 into e77546c on handshake-org:master.

@handshake-enthusiast
Copy link
Contributor

In the meantime https://github.com/Falci/hsd/blob/1fc2bc1f3ba3fab4dc81e8b93a7a049d0d409b40/docs/install.md:

hsd requires Node.js v12 or higher.

  1. Should we update it as well then?
  2. I'm not from the Node world. Does EOL mean that there is a strong reason to stop testing against it?
  3. Should https://github.com/Falci/hsd/blob/1fc2bc1f3ba3fab4dc81e8b93a7a049d0d409b40/.github/workflows/docs.yml#L14 be updated as well?

@handshake-enthusiast
Copy link
Contributor

I see that it's ci(tests), but still:

  1. Should package.json and package-lock.json be updated as well?
  "engines": {
    "node": ">=8.0.0"
  },

For me it's quite confusing that something is being tested against newest versions while still allowing old versions where the installation likely fails.

@handshake-enthusiast
Copy link
Contributor

A recent issue #714 (comment) which is related to my comment above.

@handshake-enthusiast
Copy link
Contributor

https://handshake.org/download/ says:

Node.js version 10+ is required to run: Execute hsd-3.0.1/hsd/bin/hsd

(source code)

@pinheadmz
Copy link
Member

All good points @handshake-enthusiast I see from nodejs website that v12 is EOL in just a few days! (April 30)

So it does seem prudent to update all the places you found, 14 is the new minimum and that should be reflected in package.json, the docs, and all the CI scripts.

@nodech
Copy link
Contributor

nodech commented Apr 27, 2022

You could just add new version on the matrix:

        node-version: [12.x, 14.x, 16.x, 18.x]

In the meantime https://github.com/Falci/hsd/blob/1fc2bc1f3ba3fab4dc81e8b93a7a049d0d409b40/docs/install.md:

hsd requires Node.js v12 or higher.

  1. Should we update it as well then?

  2. I'm not from the Node world. Does EOL mean that there is a strong reason to stop testing against it?

  3. Should https://github.com/Falci/hsd/blob/1fc2bc1f3ba3fab4dc81e8b93a7a049d0d409b40/.github/workflows/docs.yml#L14 be updated as well?

  4. Should package.json and package-lock.json be updated as well?

  1. I believe we don't. It just needs to state what's the minimum version supported. (If someone wants to use older machine/node after EOL it's their choice)
  2. I believe we should test it against minim required version as well, because if it ever breaks we will see it in the tests and maybe fix it or maybe drop the support altogether.
  3. That is only generating documentation, so it does not matter.
  4. Yes, we need to update package.json to show that hsd needs at least nodejs v12.

@Falci
Copy link
Member Author

Falci commented May 1, 2022

Ok, here's the recent changes:

  • added 12x back to test table;
  • updated engine to >= 12.x
  • build docs using 16.x

@nodech nodech merged commit f28fb9e into handshake-org:master May 3, 2022
handshake-enthusiast added a commit to handshake-enthusiast/handshake-web that referenced this pull request May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants