Skip to content

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Jul 22, 2023

What's the problem this PR addresses?

While working on the new website, I find myself in a situation where I need to access Yarn's cli instance (I want to use it to tokenize the examples on the website). This is difficult at the moment, as all parts of the CLI initialization are entangled together.

How did you fix it?

I extracted our initialization steps into separate functions, each called in sequence. The one breaking change I made is that --cwd is now only accepted as very first argument of the command line (this is needed to get rid of the recursive execution which caused various issues) - if it isn't, Yarn will throw an error explaining the problem.

Checklist

  • 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 arcanis mentioned this pull request Jul 22, 2023
3 tasks
@arcanis arcanis merged commit 981d5bb into master Jul 23, 2023
@arcanis arcanis deleted the mael/export-cli branch July 23, 2023 09:25
@paul-soporan paul-soporan mentioned this pull request Jul 24, 2023
3 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2023
**What's the problem this PR addresses?**
<!-- Describe the rationale of your PR. -->
<!-- Link all issues that it closes. (Closes/Resolves #xxxx.) -->

Follow-up to #5600.

This PR removes `ignoreCwd` because the option was only a broken
workaround that doesn't have a use anymore and even still leads to bugs
(try running `yarn --cwd=packages exec pwd` on `master`).

**How did you fix it?**
<!-- A detailed description of your implementation. -->

Removed it as an option but still kept it as an ignored env variable as
it's set by the parent process which could be anything (including a Yarn
version that still sets it).

Tests will come in a future PR.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [X] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [X] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [X] I will check that all automated PR checks pass before the PR gets
reviewed.
@merceyz
Copy link
Member

merceyz commented Jul 24, 2023

Note that this breaks create-react-app

Usage Error: The --cwd option is ambiguous when used anywhere else than the very first parameter provided in the command line, before even the command path

$ yarn add [--json] [-F,--fixed] [-E,--exact] [-T,--tilde] [-C,--caret] [-D,--dev] [-P,--peer] [-O,--optional] [--prefer-dev] [-i,--interactive] [--cached] [--mode #0] ...

Aborting installation.
  yarnpkg add --exact react react-dom react-scripts cra-template-typescript --cwd /tmp/tmp.fkuayeuy0I/my-cra-ts has failed.

https://github.com/yarnpkg/berry/actions/runs/5647321283/job/15297186950#step:5:43

Why aren't we letting Clipanion do the CLI argument parsing so that this still works?

@paul-soporan
Copy link
Member

Why aren't we letting Clipanion do the CLI argument parsing so that this still works?

We are, but when it detects --cwd it throws (in BaseCommand#validateAndExecute, after clipanion parses it) because that's the most correct thing to do.

Unfortunately, as I described in #5595 (comment), --cwd anywhere other than the first argument is ambiguous, if we want it to actually do the right thing and use the configuration from the target folder.

Imagine yarn foo --cwd=./bar, where ./bar is a separate project that loads a plugin that declares the foo command.

How can we know whether the command is supposed to run the foo script with the --cwd=./bar argument in the current project or whether it's supposed to run the foo command in the ./bar project?

We can't, due to the dynamic nature of the CLI. Because of this, we have 2 options:

  • we either only allow --cwd as the first argument which is never ambiguous
  • we always treat --cwd as an argument for the CLI even if it's part of a proxy, which is wrong.

Unfortunately we can't just go in ./bar and see whether there's an actual plugin declaring a foo command, as that would lead to different behavior depending on what ./bar contains.

There's also a third option (that I described in that comment):

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... ¯(ツ)/¯

It might be enough to fix CRA.

arcanis added a commit that referenced this pull request Jul 28, 2023
**What's the problem this PR addresses?**
<!-- Describe the rationale of your PR. -->
<!-- Link all issues that it closes. (Closes/Resolves #xxxx.) -->

The `--cwd` argument is currently resolved using `realpathSync`.
This behavior was introduced in
#373 as a workaround for a bug
caused by symlinks and the recursive `--cwd` logic.

**How did you fix it?**
<!-- A detailed description of your implementation. -->

Removed the unnecessary `realpath` calls. They are no longer needed due
to the removal of the recursive logic in
#5600.

I've also added a few tests, I'll add the rest from
#5595 in a future PR.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [X] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [X] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [X] I will check that all automated PR checks pass before the PR gets
reviewed.

---------

Co-authored-by: Maël Nison <[email protected]>
@arcanis arcanis mentioned this pull request Sep 12, 2023
3 tasks
arcanis added a commit that referenced this pull request Sep 13, 2023
**What's the problem this PR addresses?**

- CRA broke when [we started required that `--cwd` be put before any
other
argument](#5600 (comment)).
CRA is mostly deprecated / unmaintained by now, but it has a decent
amount of downloads (more than `create-next-app`), so it doesn't cost us
much to special-case a fix for the v4 that we can then drop in v5.

- When a machine had a hot cache for package X in both cache versions A
and B (each variant having its own checksum), when migrating a project
cache version from A to B, Yarn would mistakenly try to validate the
variant B using the checksum from variant A.

- We already have a mechanism to tolerate checksum changes when going
from one cache key to the next, but before
#5564 we used to retrieve the cache
key from the file name, whereas we now retrieve it from the lockfile
instead. A branch of code that relied on the assumption that the cache
key could be checked later became invalid, hence the problem.

**How did you fix it?**

- The `--cwd` flag is now allowed (as a special case) at the end of
`yarn add`.

- I refactored the "is this locator compatible with the current cache
key" function outside of `getLocatorPath`.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants