fix(cli): rework --cwd flag

[paul-soporan]

What's the problem this PR addresses?

The --cwd flag is incredibly buggy and its logic is very convoluted.
There are many cases where it either doesn't work at all or it leads to infinite loops.

In addition, having it implemented inside main.ts before running any commands makes it impossible for it to have any effect on nested invocations (e.g. yarn workspace).

Also, the process.chdir isn't actually needed and only overcomplicates things. All other commands that change the cwd just update context.cwd, which is the only thing needed for subsequent commands to use the right cwd.

Fixes #2346.
Fixes #4908.
Closes #4997 (supersedes).

How did you fix it?

Completely reworked the implementation to use BaseCommand.prototype.validateAndExecute instead, ensuring that all possible cases are handled correctly (I also added a lot of tests).

It is technically a breaking change because of the removal of ignoreCwd, but in practice that option was only a broken workaround that shouldn't have been used by users directly anyways, so I've just marked it as patch since it's also a very useful fix that should be backported to a last 3.x release.

There's a question remaining: Should repeated --cwd flags compose or overwrite each other?
If we treat it as a simple string option, the last occurrence will overwrite the previous ones, but if we treat it as an array, we can join all of the segments.

I'm not sure what the convention is across tools, but supporting composition doesn't really provide any benefits and also feels a bit unexpected compared to overwriting.

Checklist

• I have read the Contributing Guide.

• I have set the packages that need to be released for my changes to be effective.

• I will check that all automated PR checks pass before the PR gets reviewed.

[arcanis]

That seems something between a bug and a breaking change: it won't read the content from the .yarnrc.yml in the target folder anymore, same with everything else we do before actually running the command (like the Configuration.find calls).

[arcanis]

That said, perhaps we can move all this logic inside validateAndExecute, rather than keep it inside main 🤔

[paul-soporan]

🤔 You're right, but that was always the case for the other commands that modify the cwd (e.g. yarn ./foo [...], yarn workspace, yarn workspaces foreach). I'd agree it's a bug, but at least this PR makes it consistent across all commands 😄

The things affected by this are yarnPath, enableTelemetry and plugins (which will be ignored in the target --cwd).

yarnPath and telemetry logic could be moved to BaseCommand, but the CLI needs to know which plugins to load before it can run.

In this case, I'd rather have both yarn --cwd foo and yarn ./foo be broken but do the same thing. It would be weirdly inconsistent for one of them to use the right path and load the right plugins while the other doesn't.

[arcanis]

I needed to make changes to main for different reasons, and may have a different fix for this issue. Can you check #5600 and let me know if you find it reasonable ?

[paul-soporan]

Edit: Wrote this before fully processing your other comment. At the time I thought it was still possible to just tweak my PR to move the main logic to validateAndExecute, but now I see that there's a bit of a chicken and the egg problem and I understand why you thought of the "only specify it as the first argument" idea (which seemed quite unusual when I originally read it):

When running yarn foo --cwd=./bar, how can you know whether you're running the foo command in ./bar or the foo script with the --cwd=./bar argument if the foo command is only defined inside a plugin in the ./bar folder?
You can't, and indeed a solution would be that hacky manual argv first-argument check.

The more I think about everything, the more reasonable your solution sounds. I mentioned in my other comment that the other cwd-setting commands have the same problem, but the only one that truly does is yarn ./foo, because yarn workspace and yarn workspaces foreach are guaranteed to use a cwd in the same project, thus having the same configuration.
For yarn ./foo, we could have the same check as yarn --cwd=./foo, but at the same time I really dislike losing the ability to do yarn workspace @yarnpkg/cli ./sources exec pwd.

On the other hand only allowing it as the first argument still seems like a very weird limitation for somebody not familiar with the problem (especially in the cases where Yarn can detect it and throw an error - and people would think "if Yarn can detect that the --cwd is mispositioned why can it not just... use it and stop complaining").

Technically there's nothing limiting us from still being able to use --cwds that aren't the first argument, but only if they point to a folder that's part of the same project, via detection in validateAndExecute. And it could throw if it points to a different project, telling the user to use it as the first argument. It would still be inconsistent, but any solution we pick is going to be partially inconsistent anyways, so... ¯\(ツ)/¯

I'm not sure what I prefer TBH, there's no approach to the --cwd flag that feels right due to how dynamic Yarn is.

---

Only accepting it as the first argument feels very inconsistent and sounds like an unnecessary tradeoff that doesn't provide any benefits.

I prefer to just let BaseCommand handle it like this PR does.

(And I also strongly prefer that everything goes through clipanion unless is has a really good reason not to - manual argv checks sounds like a v1 hack that only leads to inconsistencies and bugs down the road).

Also, only accepting it as the first argument makes it incompatible to be used inside commands such as yarn workspace or yarn workspaces foreach.

this is needed to get rid of the recursive execution which caused various issues

It's not really the recursive execution that caused issues, it's just flawed logic (a big part of which I implemented myself 😄).

In any case, this PR gets rid of both and makes --cwd work in all possible cases.

[paul-soporan]

Closing in favor of your fix, I'll open separate PRs for tests, fixing yarn ./foo and removing ignoreCwd.

[merceyz]

@paul-soporan should the issues and PR you linked to be closed as well?