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

fix: pass --no-experimental-require-module for node 20.19.0+ versions #31308

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

samtsai
Copy link
Contributor

@samtsai samtsai commented Mar 17, 2025

Additional details

Introduced by node latest 20.19.0 release that also enables require(esm) by default
https://nodejs.org/en/blog/release/v20.19.0

Tested locally with added version check

Steps to test

Same fix and steps as #30730

How has the user experience changed?

No

PR Tasks

@cypress-app-bot
Copy link
Collaborator

@samtsai samtsai force-pushed the add-node-20-option branch 2 times, most recently from a826a9c to 2ca31b1 Compare March 17, 2025 16:33
@jennifer-shehane jennifer-shehane changed the title fix: add new node 20 version fix: pass --no-experimental-require-module for node 20.19.0+ versions Mar 17, 2025
@jennifer-shehane jennifer-shehane self-requested a review March 17, 2025 16:58
@jennifer-shehane
Copy link
Member

@samtsai Thanks for the PR. We're expecting this logic to be removed with Cypress 15, but seems reasonable to add for now. Going to see how the tests run.

@samtsai
Copy link
Contributor Author

samtsai commented Mar 17, 2025

@samtsai Thanks for the PR. We're expecting this logic to be removed with Cypress 15, but seems reasonable to add for now. Going to see how the tests run.

I figured since node 20 will be around for another year it'd be helpful for others

@alexsch01
Copy link
Contributor

alexsch01 commented Mar 17, 2025

same logic needs to be done for --no-experimental-detect-module - see #30084

@alexsch01
Copy link
Contributor

if (this.nodeVersion && (semver.gte(this.nodeVersion, '22.12.0') || semver.gte(this.nodeVersion, '20.19.0'))) {

Don't think this is the correct logic

wouldn't 22.0.0 get matched by this?

@samtsai
Copy link
Contributor Author

samtsai commented Mar 17, 2025

if (this.nodeVersion && (semver.gte(this.nodeVersion, '22.12.0') || semver.gte(this.nodeVersion, '20.19.0'))) {

Don't think this is the correct logic

wouldn't 22.0.0 get matched by this?

Good point, I will update it to just match within the (20.19 - 20.x)

@samtsai samtsai force-pushed the add-node-20-option branch from 00a0666 to ae43ed9 Compare March 17, 2025 19:46
@samtsai
Copy link
Contributor Author

samtsai commented Mar 17, 2025

@alexsch01 I've limited the check to 20.19.0 and greater, but not including 21.

@samtsai samtsai force-pushed the add-node-20-option branch 2 times, most recently from ae43ed9 to e2b6933 Compare March 17, 2025 20:19
@alexsch01
Copy link
Contributor

logic looks good to me

@alexsch01
Copy link
Contributor

Lets wait and see what happens from nodejs/node#57520

@samtsai
Copy link
Contributor Author

samtsai commented Mar 17, 2025

Lets wait and see what happens from nodejs/node#57520

nice, the community has responded 😄

@samtsai samtsai force-pushed the add-node-20-option branch from e2b6933 to 03cee6b Compare March 18, 2025 15:34
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR for this @samtsai! I am kind of surprised this made its way into a minor version of node as this seems quite disruptive 😅 .

Most the suggestions I have are minor. Let me know if you are able to address!

@AtofStryker AtofStryker self-requested a review March 18, 2025 18:12
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

just waiting on CI results but everything looks good on my end! Thank you!

@jennifer-shehane jennifer-shehane merged commit b303c4c into cypress-io:develop Mar 19, 2025
71 checks passed
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