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: support type: module in Node.js 16.17.0+ and 18.6.0+ #23637

Merged
merged 8 commits into from
Sep 7, 2022

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Aug 31, 2022

User facing changelog

Fix a bug where projects using Node.js 16.17+ and 18.6+ with type: module and cypress.config.ts were not working with Cypress.

Additional details

Projects with type: module using TypeScript (with a cypress.config.ts file) broke recently due to Node.js changing something on their end.

This is the exact PR to Node.js that broke everything: nodejs/node#42623. It seems even the Node.js team wasn't fully across the scope of breakage that was introduced: nodejs/node#42623 (comment)

Anyway, ts-node made some fixes in response to handle it. We use ts-node internally. I just updated the version of ts-node we ship to the latest, which has the relevant fixes. Everything seems okay now.

To test this and avoid regressions, I added two new Docker images in the cypress-docker-images repo for Node.js 16.17.0 and 18.6.0.

I then added some binary system tests to cover the Node.js versions we were not working on. I also created a branch to prove they were failing on develop (prior to this PR).

Steps to test

You could make a minimal reproduction locally like this: #22795 (comment). Then you'd build the binary and make sure it works. Or you can just see the above system tests that I added.

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 31, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Aug 31, 2022



Test summary

39662 0 3348 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 5d94e74
Started Sep 6, 2022 11:07 PM
Ended Sep 6, 2022 11:21 PM
Duration 14:25 💡
OS Linux Debian - 11.3
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@emilyrohrbough
Copy link
Member

@lmiller1990 is this related? #23393

@lmiller1990
Copy link
Contributor Author

Yep same issue @emilyrohrbough

@lmiller1990 lmiller1990 changed the title fix: support Node.js 16.17 and 18.6 fix: support type: module in Node.js 16.17.0+ and 18.6.0+ Sep 2, 2022
@lmiller1990 lmiller1990 marked this pull request as ready for review September 2, 2022 04:01
@lmiller1990 lmiller1990 requested review from flotwig, mike-plummer, a team, emilmilanov and emilyrohrbough and removed request for mike-plummer and emilmilanov September 2, 2022 04:32
@@ -36,6 +36,9 @@ export CYPRESS_CACHE_FOLDER=/tmp/CYPRESS_CACHE_FOLDER/
export npm_config_cache=/tmp/npm_config_cache/
export npm_config_package_lock=false

mkdir /tmp/npm_config_cache
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 had to add this, none of our existing binary system tests seem to actually install any node modules. These ones do, and without this line I get some random permission error when doing npm install in Docker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good find, I remember running in to that permission error while setting this up and having NFC why it was happening.

lmiller1990 and others added 2 commits September 6, 2022 09:28
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

Tested locally with latest 16 + 18 and worked like a charm 🍀

@@ -36,6 +36,9 @@ export CYPRESS_CACHE_FOLDER=/tmp/CYPRESS_CACHE_FOLDER/
export npm_config_cache=/tmp/npm_config_cache/
export npm_config_package_lock=false

mkdir $npm_config_cache
chown -R 1000:1000 $npm_config_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we do need the chown, I think this would be safer, since the uid/gid aren't guaranteed to always be 1000:1000. But maybe this is wrong, considering what happened when this line was deleted, could be some trickery afoot.

Suggested change
chown -R 1000:1000 $npm_config_cache
chown -R $(id -u):$(id -g) $npm_config_cache

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 will try this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly enough this also is not working 🤔

https://app.circleci.com/pipelines/github/cypress-io/cypress/42940/workflows/4929e8f3-cf0b-4e98-b1ae-4b5c6c83f6f0/jobs/1788721

Will ssh in and see what the output of id -u and id -g is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

circleci@ip-172-28-37-251:~$ id -u
1000
circleci@ip-172-28-37-251:~$ id -g
1000

Hmmmm.

Copy link
Contributor Author

@lmiller1990 lmiller1990 Sep 6, 2022

Choose a reason for hiding this comment

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

Oh, chown -R $(id -u):$(id -g) $npm_config_cache is just not valid code. Try running it. chown: missing operand after ‘1000:1000.

Let's do...

uid=$(id -u)
gid=$(id -g)

chown -R $uid:$gui $npm_config_cache

Copy link
Contributor

Choose a reason for hiding this comment

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

It works for me on Linux in bash and zsh. You do have a slight typo, gid => gui

This script runs in the context of the Docker container too, not the main system... and id -u and id -g work, but by default we're running as root:
image

So now I'm even more confused why 1000:1000 is needed/works here, as we are running as root already.... 🤔 🤔 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants