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.

Sign up for GitHub

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

Jump to bottom

fix: Typescript config invalid with node v22.7.0 with ESM #30099

Merged
merged 8 commits into from
Aug 27, 2024
Merged

fix: Typescript config invalid with node v22.7.0 with ESM #30099

merged 8 commits into from
Aug 27, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Aug 23, 2024 •
edited
Loading

Additional details

Cypress with Typescript and ESM stopped working with node 22.7.0. Based on the node release notes the --experimental-detect-module flag is now enabled by default which doesn't play nicely with the experimental ts_node module loader that cypress is using under the hood.

Turning it off via --no-experimental-detect-module on the child process fixes the issue.

Steps to test

Unit tests added to have some coverage in the ProjectConfigIpc and a binary system test using node 22.7.0 has been added

How has the user experience changed?

PR Tasks

@AtofStryker AtofStryker self-assigned this Aug 23, 2024
@AtofStryker AtofStryker changed the title Fix: Typescript config invalid with node v22.7.0 with ESM fix: Typescript config invalid with node v22.7.0 with ESM Aug 23, 2024
jennifer-shehane
jennifer-shehane reviewed Aug 23, 2024
View reviewed changes
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@AtofStryker
Copy link
Contributor Author

System test fails without fix https://app.circleci.com/pipelines/github/cypress-io/cypress/63656/workflows/a843fd5a-1608-413f-baac-b5c191b8b02e/jobs/2638953

@MikeMcC399
Copy link
Contributor

@AtofStryker

I'm guessing that this issue might also occur if experimental-detect-module was explicitly enabled?

If so, then maybe mention this in the changelog.

It was introduced in Node.js 20.10.0. See https://nodejs.org/docs/latest-v20.x/api/cli.html#--experimental-detect-module.

@cypress Cypress
Copy link

cypress bot commented Aug 23, 2024 •
edited
Loading

cypress    Run #56821

Run Properties:  status check passed Passed #56821  •  git commit ef6837304d: Merge branch 'develop' of github.com:cypress-io/cypress into fix/nodejs_22_7_0
Project cypress
Branch Review fix/nodejs_22_7_0
Run status status check passed Passed #56821
Run duration 18m 09s
Commit git commit ef6837304d: Merge branch 'develop' of github.com:cypress-io/cypress into fix/nodejs_22_7_0
Committer AtofStryker
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 210
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 11113
View all changes introduced in this branch ↗︎
UI Coverage  null%
  Untested elements 0  
  Tested elements 0  
Accessibility  null%
  Failed rules  0 critical   0 serious   0 moderate   0 minor
  Failed elements 0  

@AtofStryker
use environment variable to mock stubbing out of typescript install i…
36a265a 
…n order to unit test as mocking the module seems near impossible to do correctly given the context [run ci]
@AtofStryker
update changelog to include mention of experimental-detect-module
5ef8ef0
@AtofStryker
Copy link
Contributor Author

@MikeMcC399 I gave this a try in5ef8ef0

@MikeMcC399 MikeMcC399 mentioned this pull request Aug 25, 2024
Open
@MikeMcC399
Copy link
Contributor

@AtofStryker

@MikeMcC399
Copy link
Contributor

@AtofStryker

I gave this a try in5ef8ef0

  • Looking at your code, it seems you only do something different with Node.js >= 22.7.0, so the added comment is probably misplaced.

    Cypress uses its own loader for TypeScript and ES Modules to parse the Cypress config and likely will not work with the experimental-detect-module Node.js option.

  • I suggest reverting your addition to the changelog and dealing with ERR_INTERNAL_ASSERTION with Node.js >= v22.1.0 and experimental-detect-module set #30101 for Node.js < 22.7.0 separately. That would be cleaner I think.

AtofStryker
AtofStryker commented Aug 26, 2024
View reviewed changes
packages/data-context/src/util/hasTypescript.ts
@@ -1,5 +1,11 @@
export function hasTypeScriptInstalled (projectRoot: string) {
try {
// mocking this module is fairly difficult under unit test. We need to mock this for the ProjectConfigIpc unit tests
// as the scaffolded projects in the data-context package do not install dependencies related to the project.
if (process.env.CYPRESS_INTERNAL_MOCK_TYPESCRIPT_INSTALL === 'true') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out another way to test this. Not great, but at least gets us what we need

@AtofStryker AtofStryker merged commit 5a6b8c4 into develop Aug 27, 2024
83 of 84 checks passed
@AtofStryker AtofStryker deleted the fix/nodejs_22_7_0 branch August 27, 2024 13:03
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 27, 2024

Released in 13.14.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.14.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cacieprins cacieprins cacieprins approved these changes

@ryanthemanuel ryanthemanuel ryanthemanuel approved these changes

@jennifer-shehane jennifer-shehane Awaiting requested review from jennifer-shehane

Assignees

@AtofStryker AtofStryker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Typescript config file is invalid with Node 22.7
5 participants
@AtofStryker @MikeMcC399 @cacieprins @jennifer-shehane @ryanthemanuel