Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: add tsconfig option and some import errors for nodejs 18 #1644

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

Closed
ouliuquan wants to merge 4 commits into arduino:main from ouliuquan:main
Closed

fix: add tsconfig option and some import errors for nodejs 18 #1644

ouliuquan wants to merge 4 commits into arduino:main from ouliuquan:main

Conversation

Copy link

@ouliuquan ouliuquan commented Nov 7, 2022

Motivation

Fix the error for nodejs 18

Change description

fix the import package error and an Object-Types error in nodejs 18

Other information

This PR is fix for the nodejs 18, and it had been test for nodejs 16(CI build and run in Windows 11), too. So if necessary, you can merge this.

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@per1234 per1234 added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure labels Nov 7, 2022
Copy link
Contributor

It does not work for me. Steps:

% git clone https://github.com/ouliuquan/arduino-ide.git --depth 1 ./tmp \
&& nvm use 18 \
&& node -v \
&& git -C ./tmp rev-parse --short HEAD \
&& yarn --cwd ./tmp
Cloning into './tmp'...
remote: Enumerating objects: 636, done.
remote: Counting objects: 100% (636/636), done.
remote: Compressing objects: 100% (591/591), done.
remote: Total 636 (delta 57), reused 262 (delta 18), pack-reused 0
Receiving objects: 100% (636/636), 2.45 MiB | 6.12 MiB/s, done.
Resolving deltas: 100% (57/57), done.
Now using node v18.12.1 (npm v8.19.2)
v18.12.1
d15d1ef
yarn install v1.22.18
[1/5] 🔍 Validating package.json...
error arduino-ide@2.0.2: The engine "node" is incompatible with this module. Expected version ">=16.0.0 <17". Got "18.12.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Copy link
Author

It does not work for me. Steps:

% git clone https://github.com/ouliuquan/arduino-ide.git --depth 1 ./tmp \
&& nvm use 18 \
&& node -v \
&& git -C ./tmp rev-parse --short HEAD \
&& yarn --cwd ./tmp
Cloning into './tmp'...
remote: Enumerating objects: 636, done.
remote: Counting objects: 100% (636/636), done.
remote: Compressing objects: 100% (591/591), done.
remote: Total 636 (delta 57), reused 262 (delta 18), pack-reused 0
Receiving objects: 100% (636/636), 2.45 MiB | 6.12 MiB/s, done.
Resolving deltas: 100% (57/57), done.
Now using node v18.12.1 (npm v8.19.2)
v18.12.1
d15d1ef
yarn install v1.22.18
[1/5] 🔍 Validating package.json...
error arduino-ide@2.0.2: The engine "node" is incompatible with this module. Expected version ">=16.0.0 <17". Got "18.12.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Hi, @kittaakos, you need to change the arduino-ide/package.json

 "engines": {
 "node": ">=16.0.0 <17"
 },

to

 "engines": {
 "node": ">=16.0.0 <=18"
 },

I didn't send this package.json changes's PR, because I think that maybe I can't break your important limit for nodejs for some reason I hadn't know. If I can do this, I am happy to make a PR for "node": ">=16.0.0 <=18".

Copy link
Contributor

Hi, @kittaakos, you need to change the arduino-ide/package.json

No, you need to change it.

@kittaakos kittaakos added the status: changes requested Changes to PR are required before merge label Nov 7, 2022
Copy link
Contributor

I am sure there are other places to adjust things, such as the CI workflow files... First off, why do you want to change to Node.js 18?

Copy link
Author

I am sure there are other places to adjust things, such as the CI workflow files... First off, why do you want to change to Node.js 18?

I'm learning VSCode/Theia framework, designing an IDE, and arduino-ide is the best at this. nodejs has marked nodejs 18 as LTS, so I plan to build on this version. I think it might be helpful for someone with the same needs.

Copy link
Contributor

I'm learning VSCode/Theia framework, designing an IDE, and arduino-ide is the best at this. nodejs has marked nodejs 18 as LTS, so I plan to build on this version. I think it might be helpful for someone with the same needs.

Thanks for your contribution. I appreciate it. I will advise you to wait for the Node.js 18 version update.

Reason: IDE2 uses the following electron version:

"electron": "^15.3.5"

If you open IDE2 and toggle the DevTools, and print electron.versions to the Console, you will see that at runtime, the Node.js version is only 16.5.0:

Screen Shot 2022年11月07日 at 16 36 22

I will not risk accidentally using Node.js 18+ APIs at dev time that is incompatible or do not exist at runtime. I went through this once or twice, and I prefer waiting for the Theia community to do this shift, then we adjust IDE2.

Please feel free to continue with the PR and prepare IDE2 for the Node.js 18 support, but it might take months to merge.

I appreciate your understanding.

ouliuquan reacted with thumbs up emoji

Copy link
Author

You're welcome, I've been doing Electron development, and I'm well aware of the issues here, so I'm just doing my own research carefully. The Theia team is waiting for the progress of the VSCode team, and we have to wait for the progress of Theia, otherwise it will be difficult for us to maintain a version of our own without the upstream organization.

kittaakos reacted with thumbs up emoji

@kittaakos kittaakos added conclusion: declined Will not be worked on and removed status: changes requested Changes to PR are required before merge labels Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
conclusion: declined Will not be worked on topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /