fix: Use require.resolve when yarn v2 is detected #15623
Conversation
|
Thanks for taking the time to open a PR!
|
|
|
|
Not sure if this PR needs a docs update or tests |
|
@jennifer-shehane Not sure if you're the right person for this, but this has been a feature long-requested by the community. Would you be able to offer some feedback on this PR? I'd love to get this to a point where the Cypress team feels comfortable to merge as soon as possible |
|
Oh I would love to get a fix in for this. We will definitely need some tests to cover this change. Not sure exactly where/how to test this. We'd have to get some eyes on this. Do you have any ideas for testing it? |
@jennifer-shehane Sorry I don't. This is my first commit to Cypress, so I'm not sure what would be the best way. We could maybe have a unit test that mocks out both of these and tests which is called when, but I'm not sure if that's possible with the cypress test setup (would appreciate some insight from the team) |
|
@blake-transcend There's a lint error.
|
|
@jennifer-shehane ah yeah, looks like I just forgot to remove the unused import. Will fix shortly |
|
@merceyz Could I get another review? |
There was a problem hiding this comment.
@blake-transcend @merceyz thanks for working on this PR. The reasoning behind this change seems solid.
Since this is kinda complicated to test, I went ahead and created an end to end test that checks that a yarn v2 Cypress project can load a TS pluginsFile, and it fails when I run it:
/tmp/cy-yarn-v2-1617898270534/cypress/plugins/index.ts:2
exports = (on: any, config: any) => {
^
SyntaxError: Unexpected token ':'
You can run the test by using yarn test-e2e 4_yarn_v2 inside of packages/server.
Side note, I don't think this solves #8008 - that issue seems to be about using yarn v2 to resolve modules inside of browser-side code, not plugins.
| debug('resolving typescript with options %o', options) | ||
|
|
||
| const resolved = resolve.sync('typescript', options) | ||
| const resolved = require.resolve('typescript', { paths: [projectRoot] }) |
There was a problem hiding this comment.
I think the problem is that require.resolve here is still not the patched require.resolve that Yarn v2 expects, since we are running Cypress inside of Cypress's own Electron-Node instance.
Maybe the solution is to fall back to trying to use @yarnpkg/pnp to discover the path if require.resolve('typescript') fails? https://yarnpkg.com/advanced/pnpapi#resolvetounqualified
There was a problem hiding this comment.
This code actually works fine for me, my coworkers, and our CI. Could the cause be that your e2e test isn't executing cypress from yarn like you're supposed to (yarn cypress ...)? I believe that's where it injects its custom resolver. It would also probably be pretty difficult to detect which package manager you are using if you're executing outside of your package manager's environment
There was a problem hiding this comment.
Oh, I didn't realize yarn v2 required launching stuff like that, it will probably pass once the test is updated then
There was a problem hiding this comment.
@blake-transcend I updated the spec to use yarn node to launch cypress run via the CLI, but it still seems to fail with that same error. Since I'm not super familiar with how to test this (even manually), do you want to take a try at writing a test for this?
There was a problem hiding this comment.
Sure I can try with your test
|
@blake-transcend Will you have time to address the changes requested from the PR review? |
|
@jennifer-shehane blake is OOO the rest of this week and next. |
|
@jennifer-shehane i may take a stab at it this weekend. i've been going deeper into our yarn v2 cypess setup and may have a grasp of what needs to be done here. can i run these tests locally on my machine? |
|
@jennifer-shehane i opened this issue in the code-coverage repo: cypress-io/code-coverage#439 may need that to test our local setup, i also left a comment here: #6377 i updated cypress locally to 7.1 and sadly our monkey patch no longer works :/ im not sure if its worth continuing down this path here since this PR is made against an earlier version of cypress |
|
@michaelfarrell76 Let us know if you had any questions, but yah we were waiting for the requests Flotwig to be completed, to add a test, etc. If it no longer fixes the issue we'll probably need to close this PR. |
|
@jennifer-shehane whats the easiest way to run the tests locally on my machine? |
|
@michaelfarrell76 There is a Contributing Guide that covers how to contribute and get Cypress running locally here: https://github.com/cypress-io/cypress/blob/develop/.github/CONTRIBUTING.md More specifically since it looks like this is touching the server code, check the instructions for running server tests: https://github.com/cypress-io/cypress/blob/develop/packages/server/README.md It looks like there's an e2e test written in here, so follow instructions for the e2e tests under the server. |
|
I think the main reason why these tests have been failing is because this patch only works against cypress 6, and does not work for cypress 7 for some reason. I don't know why this isn't working for cypress 7 and the e2e test, but I suspect it's something to do with how cypress handles environment variables internally. I think I've reached the limit of what I will be able to understand, so I think this might need work from a dev to fix this issue |
|
Thanks for the effort put into this PR. Unfortunately right now we don't have the resources available to follow through with finishing the work here. We're going to close the PR since there's no longer any activity. If work can be continued, please feel free to open a new PR. This PR would not have solved #8008, since that issue is about using yarn v2 to resolve modules inside of browser-side code, not plugins. |
User facing changelog
Fixes support for Typescript pluginsFile when using Yarn PnP
Additional details
How has the user experience changed?
Before:
After:
[no error]
PR Tasks
cypress-documentation?