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

feat: add yarn modern pnp support #599

Merged
merged 14 commits into from
May 5, 2023
Merged

Conversation

PilotConway
Copy link
Contributor

@PilotConway PilotConway commented Sep 23, 2022

Fixes the issue with users of yarn pnp where the require of cypress happens before the checks for command and command-prefix, causing the error cannot find module 'cypress'.

yarn pnp users will be required to use the command property since those bypass the npx running of cypress and instead run through yarn which will import properly.

fixes #589
fixes #430

  • [NEW] Added examples of yarn pnp projects.
  • [CHG] Moved the cypress require statements to right before they are needed after the checks for command and command-prefix.
  • [CHG] Updated README with a note on running yarn pnp with command.
  • [CHG] build the dist files with the change.

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2022

CLA assistant check
All committers have signed the CLA.

This is required because using `yarn` command-prefix will end up running: 
`npx yarn` which doesnt resolve correctly in pnp
and results in using yarn v1 not the specified yarn. 

Using `command` will overwrite the entire command and allow `yarn dlx` 
to be used instead of `npx` which properly runs and resolves.
@PilotConway
Copy link
Contributor Author

Just updated because we found using command-prefix actually resulted in cypress being run via npx yarn which would fail when cypress versions were bumped. So changed the examples and documentation to suggest using command instead of command-prefix as that works more reliably in our tests.

@jribeiro
Copy link

jribeiro commented Jan 4, 2023

Any update on this?

@MikeMcC399
Copy link
Collaborator

This really needs some feedback from @jaffrepaul and colleagues from the Cypress organization. The existing yarn examples in this repository are using Yarn 1 (Classic) and these should stay there because this version is still important.

Adding Yarn Berry (aka Yarn v2 & v3) examples would be a great addition.

@MikeMcC399
Copy link
Collaborator

MikeMcC399 commented Jan 4, 2023

@PilotConway

My suggestion would be to separate out the changes to index.js to fix the issues mentioned in the original post and put these changes into a separate PR. Possibly these could be fast-tracked. That depends on the capacity on the cypress-io team to review and merge.

The examples would then be a separate PR. Suggestion for the examples:

  • drop Ubuntu 18 which is deprecated and add Ubuntu 22 done
  • review whether it is necessary to add legacy v9 examples at this stage, or whether new examples should only be added for v10 config structure - edit: no new v9 examples should be added
  • rebase on the current master branch done
  • use a branch name other than master for the source branch of the examples PR

I would also like to see a description in the README which makes it clearer about using the classic (v1) and current (Berry) Yarn versions. This might be in a separate PR though. Edit: this has been added

I wouldn't put any new effort into this until there is feedback from the repository owners.

@jaffrepaul jaffrepaul added bug Something isn't working type: enhancement New feature or request labels Jan 30, 2023
@MikeMcC399
Copy link
Collaborator

@PilotConway

I saw that you submitted some new commits, however you didn't write any comment. There hasn't been any comment from the Cypress.io core team either.

This comment should get your PR added to the support process.

@MikeMcC399
Copy link
Collaborator

MikeMcC399 commented May 4, 2023

@PilotConway

I ran your branch and I found that the v9 job succeeds, whilst the v12 job fails.

Suggestions

  1. Put the PR in draft status until it runs correctly. You can check the results of running the action in your own fork.
  2. Remove the v9 job and example. New v9 examples are no longer desired (see CONTRIBUTING)
  3. In the V12 example change the command to run cypress instead of cypress@9

4. Add a file examples/yarn-modern-pnp/.gitignore

```text
!.yarn/cache
!.pnp.*
```

5. Commit the cache and pnp files, otherwise you do not have a Yarn pnp environment set up.

Edit: 4. and 5. were incorrect suggestions.

Comments

  1. Cypress with Electron and Yarn pnp seems to have a compatibility issue. (see "Most NODE_OPTIONs are not supported in packaged apps" when using yarn pnp cypress#19229 regarding the error "[ERROR:node_bindings.cc(279)] Most NODE_OPTIONs are not supported in packaged apps. See documentation for more details.")

  2. Using the command parameter disables many other parameters. See Custom test command. So this may prevent its widespread use.

  3. yarn install generates the message "ESM support for PnP uses the experimental loader API and is therefore experimental" so this also needs to be considered.

@PilotConway
Copy link
Contributor Author

@MikeMcC399 Yeah, still working on a failing github action over in my repo after pulling in the latest. I was trying yesterday to see if your changes in v5 worked, but for PnP it still fails since it tried to import cypress and in pnp it's in a different location. I'm working this morning on getting the last action running then I'll update when it's ready again.

@MikeMcC399
Copy link
Collaborator

@PilotConway

With the changes that I suggested, your PR is basically working. I didn't check if it causes compatibility issues with any of the other tests though.

@PilotConway
Copy link
Contributor Author

@MikeMcC399 Thanks for those suggesstions. I'll remove that v9 then.

As for the gitignore updates, adding the cache and pnp files are only required if you want to turn on zero installs. The bug is the same for regular pnp (where it downloads on yarn install) as it is with zero installs, so I think theres no need to add the additional files for any dependencies to the repo. The difference here is that removing nodeLinker in the yarn config is what causes the bug, since the action file can no longer find cypress.

@PilotConway PilotConway marked this pull request as draft May 4, 2023 12:27
@MikeMcC399
Copy link
Collaborator

@PilotConway

As for the gitignore updates, adding the cache and pnp files are only required if you want to turn on zero installs.

Ah, I'm a relative newcomer to yarn, so sorry if I didn't work out the difference between zero installs and pnp!

@PilotConway
Copy link
Contributor Author

@MikeMcC399 No worries! It's definitely not simple getting it all setup.

This should be good to go now, the action is working on my fork and I ran the updated action to our internal application repo and it passed there as well.

@PilotConway PilotConway marked this pull request as ready for review May 4, 2023 12:49
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@PilotConway PilotConway requested a review from mschile May 5, 2023 00:30
Copy link

@mschile mschile left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution @PilotConway 🎉

@mschile mschile requested a review from chrisbreiding May 5, 2023 00:45
@MikeMcC399

This comment was marked as resolved.

@MikeMcC399
Copy link
Collaborator

@PilotConway

Congratulations on getting your first approval from the Cypress.io team!

This should help those who want to use Yarn Plug'n'Play, however the use of command: yarn dlx cypress run is really still a workaround. This PR should be followed up by further enhancements which allow Yarn Plug'n'Play to work without using the command parameter. That would allow other features like the test summary or reporting of the Cypress Cloud link after recording to work.

@MikeMcC399
Copy link
Collaborator

MikeMcC399 commented May 5, 2023

Unfortunately there is an issue in Cypress with Yarn Modern PnP. See

Running

yarn
yarn dlx cypress open

followed by selecting E2E testing results in the Cypress app results in a Bluebird issue:

-The issue is logged in cypress-io/cypress#26567.

I was able to reproduce this from scratch and it even hangs up the Cypress App on first setup, so this is quite a show-stopper for Cypress with Yarn Modern, as pnp is the default mode for any new installations.

@PilotConway PilotConway changed the title fix: fix cypress not found error for yarn pnp feat: add yarn modern pnp support May 5, 2023
@PilotConway
Copy link
Contributor Author

@MikeMcC399 Agree this is definitely a workaround to at least get it to work, and definitely think allowing the config to change npx to something else for all commands would be a huge help, but that's definitely a deeper change to the code when I was poking around.

Also in terms of running locally, its been working for us locally without any issue, but I'm seeing we were a couple versions behind (12.8.1) and something broke recently in 12.10 so that would explain it. We don't use dlx locally and use the installed version directly.

However, I think this is definitely a separate issue and the workaround can be attaching a version to the dlx command to run the version before this issue. yarn dlx [email protected] or yarn add -D [email protected] && yarn cypress to run the UI. I might try a few things over the next week and see if I dig up something, but thanks for pointing me to that issue, I'll subscribe to that post if I find anything on our end.

@MikeMcC399
Copy link
Collaborator

It seems that

yarn add [email protected] -D -E

will work around the issue for E2E testing.

@mschile mschile merged commit 73fc04a into cypress-io:master May 5, 2023
@github-actions
Copy link

github-actions bot commented May 5, 2023

🎉 This PR is included in version 5.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MikeMcC399
Copy link
Collaborator

@PilotConway

I suggest to take dlx out of the example. When it is used without a version number it uses the latest version, which is not necessarily the one defined in yarn.lock. It would be possible to add a version number, however that makes maintenance more cumbersome, as the version would need to be changed in two places when doing an update.

I tried out using just

yarn cypress run

and that looks to work well. It starts the version of Cypress already installed in the .yarn directory, which would be the one required.

What do you think?

The Yarn dlx documentation does not explain this well. I just tried it out and this is the way it works with Yarn 3.5.1.

@MikeMcC399
Copy link
Collaborator

MikeMcC399 commented May 8, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot find module 'cypress' after upgrade Error "Cannot find module 'cypress'" using Yarn Berry
7 participants