feat: Uses current release dir path, if exists #5942

jplukas · 2023-11-04T23:10:55Z

What's the problem this PR addresses?
The yarn set version command of the CLI ignores the yarnPath variable.

How did you fix it?
Changed the function invoked by the CLI in order to set the releases path to the content of the variable yarnPath, as defined in the .yarnrc.yml file if it's defined. Otherwise the default value of .yarn/releases/ holds.

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.

merceyz

Thanks for the PR!

Please add a test to https://github.com/yarnpkg/berry/blob/506ded5f5f5a89553435940c74f1d857fd685a42/packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts verifying that it works as expected.

packages/plugin-essentials/sources/commands/set/version.ts

.yarn/versions/94f0ddf0.yml

Implement fixes suggested by merceyz at yarnpkg#5942

Since the releases folder is no longer hardcoded, we must get the actual releases folder from the configuration file, in order to perform some tests.

Adds tests to validate the expected behavior of the new functionality, modifies the check function used by the tests in this suite, and alter some function calls, in order to accommodate for the changes

jplukas · 2023-11-10T13:12:24Z

packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts

+async function check(path: PortablePath, checks: {corepackVersion: string | RegExp, usePath: boolean}, releasesDir?: PortablePath) {
+  releasesDir = checks.usePath ? (releasesDir ?? ppath.join(path, `.yarn/releases`)) : undefined;
  const yarnrcPath = ppath.join(path, Filename.rc);
  const manifestPath = ppath.join(path, Filename.manifest);

  let releases: Array<string> | null;
  try {
-    releases = await xfs.readdirPromise(releasesPath);
+    releases = releasesDir ? await xfs.readdirPromise(releasesDir) : null;


Since the releases dir path is no longer hardcoded, I figured I should change the check function, in order to make the releases dir path a parameter, rather than a constant.
Also, the parameter is optional, and defaults to .yarn/releases (if the usePath parameter is true).

Also, my first ideia was to retrieve the releases dir path from the .yarnrc.yml file, but that seemed to make the tests super slow. So I made it a parameter.

jplukas · 2023-11-10T15:56:01Z

packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts

@@ -101,7 +101,7 @@ describe(`Commands`, () => {
        await xfs.writeFilePromise(ppath.join(projectDir, Filename.lockfile), ``);

        await run(`set`, `version`, `self`, {cwd: projectDir});
-        await check(projectDir, {corepackVersion: /[0-9]+\./, usePath: true});
+        await check(projectDir, {corepackVersion: /[0-9]+\./, usePath: true}, ppath.join(path, `.yarn/releases`));


In the tests where the yarnPath is set outside of the project, simply omitting the releasesDir parameter (in the new version of the check function) wouldn't work.

jplukas · 2023-11-10T15:56:24Z

packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts

@@ -136,20 +136,57 @@ describe(`Commands`, () => {
        await xfs.writeFilePromise(ppath.join(projectDir, Filename.lockfile), ``);

        await run(`set`, `version`, `self`, `--only-if-needed`, {cwd: projectDir});
-        await check(projectDir, {corepackVersion: /[0-9]+\./, usePath: true});
+        await check(projectDir, {corepackVersion: /[0-9]+\./, usePath: true}, ppath.join(path, `.yarn/releases`));


In the tests where the yarnPath is set outside of the project, simply omitting the releasesDir parameter (in the new version of the check function) wouldn't work.

jplukas · 2023-11-10T15:57:30Z

packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts

+    test(
+      `it should respect the current yarnPath, if it is defined`,
+      makeTemporaryEnv({}, {
+        env: {COREPACK_ROOT: undefined},
+      }, async ({path, run, source}) => {
+        await run(`set`, `version`, `4.0.0`);
+        await check(path, {corepackVersion: `4.0.0`, usePath: true});
+        const oldYarnPath = ppath.join(path, `.yarn/releases/yarn-4.0.0.cjs`);
+        const releasesPath = ppath.join(path, `releases_dir`);
+        await xfs.mkdirPromise(releasesPath);
+        const yarnAbsPath = ppath.join(releasesPath, `/yarn-4.0.0.cjs`);
+        const yarnPath = ppath.relative(path, yarnAbsPath);
+        await xfs.renamePromise(oldYarnPath, yarnAbsPath);
+        await xfs.writeFilePromise(ppath.join(path, Filename.rc), `yarnPath: ${yarnPath}`);
+        await run(`set`, `version`, `3.0.0`);
+        await check(path, {corepackVersion: `3.0.0`, usePath: true}, ppath.dirname(yarnAbsPath));
+      }),
+    );
+    test(
+      `it should respect the current yarnPath if defined, even when corepack is enabled and the version range is semver`,
+      makeTemporaryEnv({}, {
+        env: {COREPACK_ROOT: undefined},
+      }, async ({path, run, source}) => {
+        await run(`set`, `version`, `4.0.0`);
+        await check(path, {corepackVersion: `4.0.0`, usePath: true});
+        const oldYarnPath = ppath.join(path, `.yarn/releases/yarn-4.0.0.cjs`);
+        const releasesPPath = ppath.join(path, `releases_dir`);
+        await xfs.mkdirPromise(releasesPPath);
+        const yarnAbsPath = ppath.join(releasesPPath, `/yarn-4.0.0.cjs`);
+        const yarnPath = ppath.relative(path, yarnAbsPath);
+        await xfs.renamePromise(oldYarnPath, yarnAbsPath);
+        await xfs.writeFilePromise(ppath.join(path, Filename.rc), `yarnPath: ${yarnPath}`);
+        await run(`set`, `version`, `3.0.0`, {env: {COREPACK_ROOT: `/path/to/corepack`}});
+        await check(path, {corepackVersion: `3.0.0`, usePath: true}, ppath.dirname(yarnAbsPath));
      }),
    );


New tests, as requested in #5942 (review)

jplukas · 2023-11-10T16:00:11Z

.yarn/versions/94f0ddf0.yml

I'm not entirely sure which packages should I mark to release, or what kind of update should I mark the packages with (whether it's a patch, or minor update). Clarifications would be welcome 😄

jplukas · 2023-11-10T16:04:40Z

packages/acceptance-tests/pkg-tests-specs/sources/commands/set/version.test.ts

I changed some tests, in order for them to be compatible with the new functionality. Please let me know if this was the right decision, or if I should not have changed existing tests.

jplukas · 2023-11-26T18:23:07Z

@merceyz Hi, sorry to bother, but could you please review this again? I've made the changes you have requested.
I'm not entirely sure if pinging you here is the right course of action, please let me know if I'm being rude or disrespectful.
Thanks!

merceyz requested changes Nov 4, 2023

View reviewed changes

packages/plugin-essentials/sources/commands/set/version.ts Outdated Show resolved Hide resolved

packages/plugin-essentials/sources/commands/set/version.ts Outdated Show resolved Hide resolved

.yarn/versions/94f0ddf0.yml Outdated Show resolved Hide resolved

fernandohjunqueira added a commit to jplukas/berry that referenced this pull request Nov 6, 2023

Fix: Use ppath dirname and mark for release

c0b1850

Implement fixes suggested by merceyz at yarnpkg#5942

jplukas and others added 5 commits November 9, 2023 15:43

Uses current release dir path, if exists

32dfb11

Fix: Use ppath dirname and mark for release

e11a71a

Implement fixes suggested by merceyz at yarnpkg#5942

Modifies check function in set version tests

debd640

Since the releases folder is no longer hardcoded, we must get the actual releases folder from the configuration file, in order to perform some tests.

Adds tests to "set version" command, and modify some tests

f5ecef9

Adds tests to validate the expected behavior of the new functionality, modifies the check function used by the tests in this suite, and alter some function calls, in order to accommodate for the changes

Mark for release

688a5d4

jplukas force-pushed the feat/set-version-yarnpath branch from 7793ae4 to 688a5d4 Compare November 9, 2023 18:43

jplukas commented Nov 10, 2023

View reviewed changes

Remove duplicate test

14a22de

jplukas commented Nov 10, 2023

View reviewed changes

jplukas requested a review from merceyz November 10, 2023 16:08

Merge branch 'master' into feat/set-version-yarnpath

f3bb301

Merge branch 'master' into feat/set-version-yarnpath

f0e6a79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Uses current release dir path, if exists #5942

feat: Uses current release dir path, if exists #5942

jplukas commented Nov 4, 2023 •

edited

Loading

merceyz left a comment •

edited

Loading

jplukas Nov 10, 2023 •

edited

Loading

jplukas Nov 10, 2023

jplukas Nov 10, 2023

jplukas Nov 10, 2023

jplukas Nov 10, 2023 •

edited

Loading

jplukas Nov 10, 2023

jplukas Nov 10, 2023

jplukas commented Nov 26, 2023

feat: Uses current release dir path, if exists #5942

Are you sure you want to change the base?

feat: Uses current release dir path, if exists #5942

Conversation

jplukas commented Nov 4, 2023 • edited Loading

merceyz left a comment • edited Loading

Choose a reason for hiding this comment

jplukas Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

jplukas Nov 10, 2023

Choose a reason for hiding this comment

jplukas Nov 10, 2023

Choose a reason for hiding this comment

jplukas Nov 10, 2023

Choose a reason for hiding this comment

jplukas Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

jplukas Nov 10, 2023

Choose a reason for hiding this comment

jplukas Nov 10, 2023

Choose a reason for hiding this comment

jplukas commented Nov 26, 2023

jplukas commented Nov 4, 2023 •

edited

Loading

merceyz left a comment •

edited

Loading

jplukas Nov 10, 2023 •

edited

Loading

jplukas Nov 10, 2023 •

edited

Loading