Move from node `16` to node `>=18` #12711

paul-marechal · 2023-07-13T21:44:08Z

What it does

Update our CI/CD to run using Node.js 18.x and 20.x.

Update macaddress to include the fix for Node.js 18.x.

Closes #11220

How to test

Everything should run as usual? CI/CD should be green.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

paul-marechal · 2023-07-13T22:05:44Z

OK there's something weird going on with the CI runners and I won't have much time to troubleshoot this.

I don't get any of the errors the Windows workflow has, even when starting from a fresh clone.

The /bin/sh: 1: theia: not found errors on Ubuntu/Mac are also odd as they imply that yarn doesn't do a good job of exposing npm bins to package.json scripts? I'd need someone to try on their own Ubuntu machine to check...

Help is welcome here.

rahulgupta-acquia · 2023-07-26T10:10:03Z

@paul-marechal Could you please rebase this repo, this repo has some conflicts that needs to fixed for testing.

rahulgupta-acquia · 2023-07-26T15:31:52Z

@paul-marechal could you please suggest a way to test this build in my local ( MAC ) system,

I followed the below steps for setup https://github.com/eclipse-theia/theia/blob/master/doc/Developing.md#quick-start

git clone https://github.com/eclipse-theia/theia \
    && cd theia \
    && yarn \
    && yarn download:plugins \
    && yarn browser build \
    && yarn browser start

but after cloning and yarn install, it throws the

 $ yarn download:plugins
command not found: theia

paul-marechal · 2023-07-26T15:42:50Z

@rahulgupta-acquia well the fact that it doesn't work is the issue I was asking help with... But looking at the logs again I might have figured it out: See latest commit.

rahulgupta-acquia · 2023-07-26T15:44:34Z

@rahulgupta-acquia well the fact that it doesn't work is the issue I was asking help with... But looking at the logs again I might have figured it out: see latest commit.

yes, i checked yarn add global node-gyp is required for further steps.

paul-marechal · 2023-07-26T15:46:11Z

@rahulgupta-acquia you mean that it worked for you when using yarn global add node-gyp before doing yarn install?

rahulgupta-acquia · 2023-07-26T15:47:34Z

@rahulgupta-acquia you mean that it worked for you when using yarn global add node-gyp before doing yarn install?

No, i am facing issue at the time of yarn download;plugin.

I think my issue is not related to this PR.

rahulgupta-acquia · 2023-07-26T15:48:47Z

@paul-marechal Do you have any document or steps to test this PR in Local system (MAC)?

paul-marechal · 2023-07-26T15:50:07Z

@rahulgupta-acquia I don't have any more steps than what you found here: https://github.com/eclipse-theia/theia/blob/master/doc/Developing.md#quick-start

rahulgupta-acquia · 2023-07-26T15:50:36Z

I need to build IDE using this PR.

rahulgupta-acquia · 2023-07-26T15:52:30Z

@paul-marechal Once this CI/CD success, Do we say Theia is supported Node >=18?

paul-marechal · 2023-07-26T15:53:37Z

@rahulgupta-acquia I'd like for other contributors to check for regressions before proceeding but yes!

kittaakos · 2023-07-26T17:40:42Z

Thanks for the update. Could you please adjust the developer readme? Thanks

theia/doc/Developing.md

Line 55 in c8fb5ad

- Node.js `>= 16.14.0` and `< 17`.

paul-marechal · 2023-07-26T20:40:30Z

IP v6 addresses were being used over IP v4 but I found a way to get back to the previous behaviour using dns.setDefaultResultOrder('ipv4first')

rahulgupta-acquia

Reviewed the code changes.

rahulgupta-acquia · 2023-07-27T12:33:52Z

@paul-marechal Could you please rebase this repo.
Thanks

rahulgupta-acquia · 2023-07-31T07:38:00Z

@paul-marechal code rebase needed.

@msujew @tsmaeder @vince-fugnitto Request for review.

.github/workflows/ci-cd.yml

msujew

We need to upgrade the node version here as well. I also agree with Vince, that as long VSCode extensions are using Node 16, that we should use 16 in our testing matrix as well.

Otherwise this looks pretty good to me 👍

msujew

The changes look good to me 👍

@vince-fugnitto Do we want to include this in the upcoming release?

vince-fugnitto · 2023-08-23T11:46:37Z

@vince-fugnitto Do we want to include this in the upcoming release?

Sounds good to me!

Using Node 20.x it seems like Yarn complains about missing `node-gyp`. It will actually shortcut the first call to `yarn install` when it is missing in order to automatically install it. Instead we'll explicitly make sure that node-gyp is installed.

jcortell68 · 2023-11-01T22:55:38Z

package.json

@@ -4,17 +4,17 @@
  "version": "0.0.0",
  "engines": {
    "yarn": ">=1.7.0 <2",
-    "node": ">=16.14.0 <17"
+    "node": ">=16"


Curious: the Developing.md was updated to say developers should use >= 18.17 and < 21. But this line says >= 16. Why that discrepancy?

While we generally recommend the version range that you've mentioned, there is no technical reason for limiting it to that range. Though we know that < 16.0.0 doesn't work due to native dependencies. 18.17.0 is specifically the recommended minimum version because that's the version that our Electron version uses.

We recommend a certain version range in our documentation as we try to make sure it works for said range.

This engines.node field will have Yarn stop the build if you use anything outside the defined range, and I might have left it lax so that it's more convenient when switching between node versions for development purposes.

Most other NPM projects -including our deps- are also pretty lax with this range (click to expand).

https://github.com/websockets/ws/blob/d8dd4852b81982fc0a6d633673968dff90985000/package.json#L32

https://github.com/socketio/socket.io/blob/9a2a83fdd42faa840d4f11fd223349e5d8e4d52c/package.json#L92

https://github.com/jprichardson/node-fs-extra/blob/a277cbbdf630424482b1e7418749ccaac0db98fe/package.json#L6

https://github.com/RogerDeng/drivelist/blob/6b79bf74a40e19bc69a561b978067807060192c4/package.json#L36

https://github.com/facebook/react/blob/faa65778b7c277e0020b8bb1fc172e11a863c0b7/packages/react/package.json#L38

https://github.com/Axosoft/nsfw/blob/13a84d97c0d778535bbc3baebcc6dc1216a26325/package.json#L22

https://github.com/webpack/webpack/blob/87660921808566ef3b8796f8df61bd79fc026108/package.json#L113

etc

But we could move the lower bound along the minimum recommended version.

At the very minimum we need to update this range based on the kind of constructs we use from NodeJS: we can't use new Node 18 syntax and claim that we require anything below Node 18 to run our code base. Although syntax-wise we're dependent on our TypeScript compilation target: No matter what TS syntax we use tsc will "dumb it down" for us in the output JS based on our configs. We're currently targeting ES2019 and it seems like Node 16 supports it very well.

In the long run it's in everyone's best interest to always use the latest Node LTS, but some might still lag behind so the range can accommodate for it somewhat.

Now I remember: Had I updated the range to >=18 for that release then all users of the framework would have had build issues because they most likely were still running on Node 16 at that point. So instead of breaking everyone on release I kept it that way so people can update when they're ready. It's not like the project will stop working on Node 16 right away :)

jcortell68 · 2023-11-02T13:04:50Z

Thanks guys. That was very enlightening. So if I had to summarize the sum of the messages and maybe read between the lines a little, this is what I think is being said:

At this point in time:

there are no issues building Theia with Node 16
there are no known issues running Theia with Node 16 (but in theory there could be issues lurking or unreported)
the version of Node that the version of Electron Theia uses is Node 18.17
the Theia project is not actively trying to stay compatible with Node 16, and thus to avoid potential breakage in the future, adopters should use v18 or greater
in general, using the latest Node LTS is ideal

paul-marechal added ci issues related to CI / tests dependencies pull requests that update a dependency file potentially-breaking A change that might break some dependents conditionally labels Jul 13, 2023

paul-marechal force-pushed the mp/node-18 branch from 10d363c to 50b892f Compare July 26, 2023 14:48

paul-marechal force-pushed the mp/node-18 branch from 16fa0cf to a79c8b1 Compare July 26, 2023 15:44

rahulgupta-acquia reviewed Jul 27, 2023

View reviewed changes

paul-marechal requested review from msujew, tsmaeder and vince-fugnitto July 27, 2023 06:20

paul-marechal force-pushed the mp/node-18 branch 2 times, most recently from 483e0a2 to 73b51e2 Compare July 27, 2023 17:13

vince-fugnitto reviewed Jul 31, 2023

View reviewed changes

.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved

paul-marechal mentioned this pull request Aug 8, 2023

future: support Node 18 #11220

Closed

msujew reviewed Aug 8, 2023

View reviewed changes

paul-marechal force-pushed the mp/node-18 branch from 73b51e2 to 6c6a24d Compare August 8, 2023 13:08

kittaakos mentioned this pull request Aug 23, 2023

Stuck at loading screen #12751

Open

msujew approved these changes Aug 23, 2023

View reviewed changes

paul-marechal added 13 commits August 25, 2023 09:52

update deps for node 18.x

e06814c

update breaking changes

fb54b4f

update workflows

b452b27

make sure node-gyp is installed

6a230f1

Using Node 20.x it seems like Yarn complains about missing `node-gyp`. It will actually shortcut the first call to `yarn install` when it is missing in order to automatically install it. Instead we'll explicitly make sure that node-gyp is installed.

update logger tests

5330365

update docs

84f121f

force ipv4 and improve ipv6 support

0e8482f

fix backend address messaging

73cbda3

update backend cli options

b0dc795

update runtime policy

8b83fa0

fix backend cli

a2247fa

use node 18.x in gitpod image

eb57f1e

keep using node 16.x in workflow

493c4f0

vince-fugnitto force-pushed the mp/node-18 branch from bb72a2d to 493c4f0 Compare August 25, 2023 13:53

vince-fugnitto merged commit 2d31490 into master Aug 28, 2023
12 checks passed

vince-fugnitto deleted the mp/node-18 branch August 28, 2023 13:19

github-actions bot added this to the 1.41.0 milestone Aug 28, 2023

jcortell68 reviewed Nov 1, 2023

View reviewed changes

t1m0thyj mentioned this pull request Nov 16, 2023

Fix Theia Docker build for Node 20 zowe/zowe-sample-dockerfiles#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from node `16` to node `>=18` #12711

Move from node `16` to node `>=18` #12711

paul-marechal commented Jul 13, 2023 •

edited

Loading

paul-marechal commented Jul 13, 2023 •

edited

Loading

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023 •

edited

Loading

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

kittaakos commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia left a comment

rahulgupta-acquia commented Jul 27, 2023

rahulgupta-acquia commented Jul 31, 2023

msujew left a comment

msujew left a comment

vince-fugnitto commented Aug 23, 2023

jcortell68 Nov 1, 2023

msujew Nov 1, 2023

paul-marechal Nov 1, 2023

paul-marechal Nov 1, 2023

jcortell68 commented Nov 2, 2023 •

edited

Loading

Move from node 16 to node >=18 #12711

Move from node 16 to node >=18 #12711

Conversation

paul-marechal commented Jul 13, 2023 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal commented Jul 13, 2023 • edited Loading

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023 • edited Loading

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

rahulgupta-acquia commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

kittaakos commented Jul 26, 2023

paul-marechal commented Jul 26, 2023

rahulgupta-acquia left a comment

Choose a reason for hiding this comment

rahulgupta-acquia commented Jul 27, 2023

rahulgupta-acquia commented Jul 31, 2023

msujew left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Aug 23, 2023

jcortell68 Nov 1, 2023

Choose a reason for hiding this comment

msujew Nov 1, 2023

Choose a reason for hiding this comment

paul-marechal Nov 1, 2023

Choose a reason for hiding this comment

paul-marechal Nov 1, 2023

Choose a reason for hiding this comment

jcortell68 commented Nov 2, 2023 • edited Loading

Move from node `16` to node `>=18` #12711

Move from node `16` to node `>=18` #12711

paul-marechal commented Jul 13, 2023 •

edited

Loading

paul-marechal commented Jul 13, 2023 •

edited

Loading

paul-marechal commented Jul 26, 2023 •

edited

Loading

jcortell68 commented Nov 2, 2023 •

edited

Loading