fix: correctly resolve dependencies for CT onboarding when using Yarn Plug n Play

[lmiller1990]

• Closes Dependencies are not detected during onboarding when using Yarn 3 #25960

Additional details

Correctly detect installed modules for projects using Yarn 3 and Plug n Play.

Steps to test

Make a Yarn 3 project with Vue and Vite (or whatever deps you like).

mkdir yarn-3-repro
cd yarn-3-repro
yarn set version berry
yarn init -y
yarn add vite vue cypress                                                                  

Try setup CT with the current cypress (just do yarn cypress open). It does not detect your deps.

Now use this branch - it detects them correctly! 🎉

How has the user experience changed?

Using Yarn 3 to onboard does not give the impression something is broken.

PR Tasks

• Have tests been added/updated?
• Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?

[lmiller1990]

Might be worth reading https://yarnpkg.com/features/pnp for more context.

[lmiller1990]

.pnp.cjs is regenerated by Yarn every time you install a module. We only want to require it once, since the trick Yarn PnP uses is to dynamically monkeypatch module and do some other weird hacks to facilitate the various features of PnP. That is what setup() does.

It's a bit of a hack (Yarn 3, I mean, not what I'm doing here - this is how they recommend you load the .pnp.cjs if you are doing it manually). Yarn 3 does run really fast, though, because of this... although Yarn 1 -> Yarn 2.x + PnP is a heck of a lift for a lot of code bases and projects, which is why many people are stuck on v1 (including FB, the (original) authors of Yarn).

It wouldn't matter if we executed this block it every time - it would just be a bit of pointless overhead. It would be next to negligible, since Node.js caches require calls anyway, but this is cleaner. We do need to execute it once for each new project (think global mode - you can switch projects, each having a different .pnp.cjs).

[cypress]

29 flaky tests on run #45425 ↗︎

0

27259

1307

0

29

Details:

Merge branch 'develop' into issue-25960
Project: cypress	Commit: 948de1bdf0
Status: Passed	Duration: 19:05 💡
Started: Apr 10, 2023 11:50 PM	Ended: Apr 11, 2023 12:09 AM

  e2e/origin/user_agent_override.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
user agent override > persists modified user agent after cy.go		Output Video

  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test		Artifacts
... > correctly returns currentRetry		Output Video
... > correctly returns currentRetry		Output Video
... > correctly returns currentRetry		Output Video

  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test		Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse		Output

  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test		Artifacts
... > correctly returns currentRetry		Output
... > correctly returns currentRetry		Output
... > correctly returns currentRetry		Output

  create-from-component.cy.ts • 1 flaky test • app-e2e

View Output Video

Test		Artifacts
... > runs generated spec		Output Screenshots Video

The first 5 flaky specs are shown, see all 18 specs in Cypress Cloud.

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[lmiller1990]

Sent the author a patch stefanpenner/resolve-package-path#62

[lmiller1990]

Problem is the module is installed in the same directory as the project, which in our case it isn't - the module is in the binary.

[mike-plummer]

Amazing job tracking this down 🤯

I have just a couple style comments and one potential edge case, but none of them are real blockers and can be addressed as follow-ons if necessary so this can make the release

[mike-plummer]

omg, never would have figured this one out. Very nice!

[mike-plummer]

Very minor, but we could simplify this map to just be a Map<string, boolean>. Do you foresee any other fields being tracked that would require an object value?

[lmiller1990]

I had this thought too, my reasoning was the property usesYarnPnP makes it more obvious what this is used for. Could go either way, I wish TS had a feature where you could label the value, eg Map<string, usesPnP as boolean> or something...

[mike-plummer]

Maybe I'm missing an edge case, but couldn't we just use the require(require.resolve(packageFilePath)) logic in both cases? PnP handles if the module is zipped, and if we aren't in PnP it will be a flat file that should be normally require-able

[lmiller1990]

I think there was an edge case where this doesn't work, let me double check.

[mike-plummer]

What about a scenario where a Cypress project is nested into a subdirectory of a yarn project? Do we need to traverse upwards to find the first .pnp.cjs we encounter?

Theres a method on the module module that seems to do this for us in a PnP context (find .pnp.cjs, load, and return pnpapi (which we can ignore))

[lmiller1990]

Ah, forgot the nested edge case, I will verify what happens and address using this if required. Yarn has a ton of APIs for this, as you pointed out, we can probably use one.

[lmiller1990]

Painful, but for this API to exist you need to be in a PnP context... which you get by requiring the file in the first place 🤦

I think we need to manually traverse up, lucky we've already got some code that does that, isRepositoryRoot.

[lmiller1990]

@mike-plummer

1. I addressed the "find pnp.cjs recursively in monorepo" comment, this commit => 775314a
2. I addressed "can't we just require.resolve no matter what here => 47b41bd

@astone123 can you take a look? According to #26033 "There are some dependencies that we can't resolve with require.resolve, because of one of the following reasons:"... but this seems to not be the case anymore, I'm using require.resolve on all the things and they all pass. I wonder if resolve-package-path does not have the same limitations?

[astone123]

@lmiller1990 resolve-package-path does not have those same limitations, which is why I introduced it in #26033.

I think that this will work around those limitations though because the difference here is that we've used resolve-package-path to get the full path of the package file, rather than trying to resolve with just the module name.

The issue before was that we were trying to do

require.resolve('my-module')

which will only work if the package file either doesn't specify any exports, or does and includes package.json. But in this case we'd be doing

require.resolve('path/to/my-module/package.json')

so we shouldn't have issues

[sfsepark]

@lmiller1990
Unfortunately, even after 12.10.0 deployment, there is still an error in putting the reporter in the settings in the pnp environment. I thought it would be solved after this patch is applied, but it's very unfortunate.

Is there a solution to this? Still, is there a solution other than changing the cypress internal code calling the reporter?

ref issue : #18922

Error loading the reporter: cypress-multi-reporter

We searched for the reporter in these paths:

 - /Users/user/Documents/GitHub/.../project/cypress-multi-reporter
 - /Users/user/Documents/GitHub/.../project/node_modules/cypress-multi-reporter

Learn more at https://on.cypress.io/reporters

Error: Cannot find module '/Users/user/Documents/GitHub/.../project/node_modules/cypress-multi-reporter'
Require stack:
- /Users/user/Library/Caches/Cypress/12.10.0/Cypress.app/Contents/Resources/app/packages/server/lib/reporter.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:940:15)
    at n._resolveFilename (node:electron/js2c/browser_init:249:1105)
    at PackherdModuleLoader._tryResolveFilename (evalmachine.<anonymous>:1:737331)

[lmiller1990]

@sfsepark I'll reply in #18922

[chrisbreiding]

Looks like this may have caused a regression: #26676

[lmiller1990]

@chrisbreiding I think you are right. We might be best to simply revert that commit until we've got a fix - I took a quick look at the new issue, #26676, but I don't think it's a straight forward fix.