Skip to content

Commit 07d90c4

Browse files
arcanismerceyz
authored andcommitted
Fixes the optional check depending on the order the tree is traversed (#5840)
**What's the problem this PR addresses?** Depending on the order we traversed the dependency tree, it could happen that we'd see a package as optional before seeing the other cases where it was not optional. Because marking the package as "not optional" occurred after the "has this package been traversed before?" check, we were never updating its status from "optional" to "not optional". Fixes #5827 **How did you fix it?** We now check for optionality regardless of whether the package has been seen or not before. I added an order-dependent dragon test to try to prevent regressions. **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. (cherry picked from commit 7aa2359)
1 parent 4ea4cab commit 07d90c4

3 files changed

Lines changed: 81 additions & 3 deletions

File tree

.yarn/versions/0d12a741.yml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/core": patch
4+
5+
declined:
6+
- "@yarnpkg/plugin-compat"
7+
- "@yarnpkg/plugin-constraints"
8+
- "@yarnpkg/plugin-dlx"
9+
- "@yarnpkg/plugin-essentials"
10+
- "@yarnpkg/plugin-exec"
11+
- "@yarnpkg/plugin-file"
12+
- "@yarnpkg/plugin-git"
13+
- "@yarnpkg/plugin-github"
14+
- "@yarnpkg/plugin-http"
15+
- "@yarnpkg/plugin-init"
16+
- "@yarnpkg/plugin-interactive-tools"
17+
- "@yarnpkg/plugin-link"
18+
- "@yarnpkg/plugin-nm"
19+
- "@yarnpkg/plugin-npm"
20+
- "@yarnpkg/plugin-npm-cli"
21+
- "@yarnpkg/plugin-pack"
22+
- "@yarnpkg/plugin-patch"
23+
- "@yarnpkg/plugin-pnp"
24+
- "@yarnpkg/plugin-pnpm"
25+
- "@yarnpkg/plugin-stage"
26+
- "@yarnpkg/plugin-typescript"
27+
- "@yarnpkg/plugin-version"
28+
- "@yarnpkg/plugin-workspace-tools"
29+
- "@yarnpkg/builder"
30+
- "@yarnpkg/doctor"
31+
- "@yarnpkg/extensions"
32+
- "@yarnpkg/nm"
33+
- "@yarnpkg/pnpify"
34+
- "@yarnpkg/sdks"

packages/acceptance-tests/pkg-tests-specs/sources/dragon.test.js

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,4 +682,48 @@ describe(`Dragon tests`, () => {
682682
},
683683
),
684684
);
685+
686+
test(
687+
`it should pass the dragon test 13`,
688+
makeTemporaryEnv(
689+
{
690+
workspaces: [
691+
`pkg-a`,
692+
`pkg-b`,
693+
],
694+
},
695+
async ({path, run, source}) => {
696+
// This dragon test represents the following scenario:
697+
//
698+
// .
699+
// ├── pkg-a/
700+
// │ └── no-deps-failing (optional)
701+
// └── pkg-b/
702+
// └── no-deps-failing (not optional)
703+
//
704+
// Depending on the order of traversal we may end up marking no-deps-failing
705+
// as being traversed, and skip all future traversals. If we're not being
706+
// careful this may cause setting the "not optional" flag to be skipped as
707+
// well, making Yarn believe that no-deps-failing is optional when it's not.
708+
709+
await xfs.mkdirPromise(`${path}/pkg-a`);
710+
await xfs.writeJsonPromise(`${path}/pkg-a/package.json`, {
711+
name: `pkg-a`,
712+
optionalDependencies: {
713+
[`no-deps-failing`]: `1.0.0`,
714+
},
715+
});
716+
717+
await xfs.mkdirPromise(`${path}/pkg-b`);
718+
await xfs.writeJsonPromise(`${path}/pkg-b/package.json`, {
719+
name: `pkg-b`,
720+
dependencies: {
721+
[`no-deps-failing`]: `1.0.0`,
722+
},
723+
});
724+
725+
await expect(run(`install`)).rejects.toThrowError(`no-deps-failing@npm:1.0.0 couldn't be built successfully`);
726+
},
727+
),
728+
);
685729
});

packages/yarnpkg-core/sources/Project.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2018,14 +2018,14 @@ function applyVirtualResolutionMutations({
20182018
};
20192019

20202020
const resolvePeerDependenciesImpl = (parentDescriptor: Descriptor, parentLocator: Locator, peerSlots: Map<IdentHash, LocatorHash>, {top, optional}: {top: LocatorHash, optional: boolean}) => {
2021+
if (!optional)
2022+
optionalBuilds.delete(parentLocator.locatorHash);
2023+
20212024
if (accessibleLocators.has(parentLocator.locatorHash))
20222025
return;
20232026

20242027
accessibleLocators.add(parentLocator.locatorHash);
20252028

2026-
if (!optional)
2027-
optionalBuilds.delete(parentLocator.locatorHash);
2028-
20292029
const parentPackage = allPackages.get(parentLocator.locatorHash);
20302030
if (!parentPackage)
20312031
throw new Error(`Assertion failed: The package (${structUtils.prettyLocator(project.configuration, parentLocator)}) should have been registered`);

0 commit comments

Comments
 (0)