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

Update nodejs version to 20.10.0 #23980

Merged
merged 11 commits into from
Dec 16, 2023
Merged

Update nodejs version to 20.10.0 #23980

merged 11 commits into from
Dec 16, 2023

Conversation

DanielFran
Copy link
Member

Fix #23882


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@DanielFran DanielFran changed the title Update nodejs version to 20.8.1 Update nodejs version to 20.9.0 Oct 24, 2023
@mshima
Copy link
Member

mshima commented Oct 24, 2023

Tests are failing without been reported.

@mshima
Copy link
Member

mshima commented Oct 24, 2023

At current state I don't think it will succeed.
Loaders support at node 20 had a big behavior change.

@mraible
Copy link
Contributor

mraible commented Oct 25, 2023

@mshima All tests pass. Do you think it's OK to merge?

@mshima
Copy link
Member

mshima commented Oct 25, 2023

@mraible tests are not passing. npm test is failing silently.

@DanielFran
Copy link
Member Author

DanielFran commented Oct 25, 2023

@mshima Are you referring to that issue?
image

Seems to be missing the cli tests

@DanielFran DanielFran marked this pull request as draft October 25, 2023 09:53
@mshima
Copy link
Member

mshima commented Oct 25, 2023

Success should report:
image

Locally the failure is verbose.

@mshima mshima closed this Oct 25, 2023
@mshima mshima reopened this Oct 25, 2023
@mshima mshima closed this Oct 26, 2023
@mshima mshima reopened this Oct 26, 2023
@mshima mshima closed this Oct 28, 2023
@mshima mshima reopened this Oct 28, 2023
@mraible
Copy link
Contributor

mraible commented Oct 30, 2023

@mshima Do you think the CI failures are caused by the Node upgrade?

@mshima
Copy link
Member

mshima commented Oct 30, 2023

@mraible it's related to how node handles loaders (adds support to typescript and esm mocking).
Typescript loader works as expected, but esm mocking requires global state to keep the mocking state.

Loaders in node 18 and prior versions are considered experimental and are rc in node 20.

Loaders in node 20 runs in a different thread. The mocking state should be kept in user thread, but the mocking logic is done at the loaders thread.
The communication between the loaders thread and user thread should be reworked to use the thread channels.
This is what needs to be done: testdouble/quibble@v0.6.17...v0.9.0.

We are using https://github.com/node-loaders/loaders/tree/main/workspaces/mock loader for mocking.
It should be updated to give support to node 20.

It should be possible to rework esm mocking since is used in 2 files only.

@mraible
Copy link
Contributor

mraible commented Oct 30, 2023

@mshima Will it be possible to provide Node 18 and Node 20 support, or should we make Node 20 the baseline?

@mshima
Copy link
Member

mshima commented Oct 30, 2023

Support both.

@mraible mraible added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $300 https://www.jhipster.tech/bug-bounties/ labels Nov 3, 2023
@mraible
Copy link
Contributor

mraible commented Nov 3, 2023

I'm gonna keep adding bug bounties until someone stops me. 😜

@DanielFran
Copy link
Member Author

@mshima It seems now that the tests break now with details

@mshima
Copy link
Member

mshima commented Nov 7, 2023

Yes, that's the error.
Node 20 doesn't need to re-execute node apps passing the loader as argument or environment option. It can be registered at runtime.
I was thinking that it was a mocha misbehavior with unhandled exception but now seems to be in wrapper.

@DanielFran DanielFran changed the title Update nodejs version to 20.9.0 Update nodejs version to 20.10.0 Nov 22, 2023
@mraible
Copy link
Contributor

mraible commented Nov 29, 2023

Any updates that can help move this one along?

@mshima
Copy link
Member

mshima commented Dec 12, 2023

I will try with vitest.

@mshima
Copy link
Member

mshima commented Dec 13, 2023

I am doing some tests with vitest and is not a good alternative for the main generator.
This was previously considered in #22108 (comment).

vitest uses vite pipeline which brings a big overhead over mocha.
We are using vitest in blueprints which has much lower requirements.

JDL tests takes ~7 seconds using mocha and ~32 seconds using vitest in a M1 processor.

We may consider node:test as a mocha alternative in the future.

@mshima
Copy link
Member

mshima commented Dec 13, 2023

There is a script to convert tests to vitest here.
If anyone wants to try.

I will switch the mocking lib to quibble and required node 18.19.0 for tests.

@mshima mshima closed this Dec 16, 2023
@mshima mshima reopened this Dec 16, 2023
@DanielFran DanielFran marked this pull request as ready for review December 16, 2023 09:48
@mshima mshima merged commit 7a632b8 into main Dec 16, 2023
109 of 119 checks passed
@mshima mshima deleted the skip_ci-node_20.8.1 branch December 16, 2023 13:04
@deepu105 deepu105 added this to the 8.2.0 milestone Mar 20, 2024
@mshima
Copy link
Member

mshima commented Jul 16, 2024

@DanielFran bounty for related PRs claimed https://opencollective.com/generator-jhipster/expenses/211709

@DanielFran
Copy link
Member Author

@mshima approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
$$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ pr: skip-closing-issues-check theme: CI builds theme: jhipster-internals $300 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Node.js 20
4 participants