[Bug?]: Dependency considered optional but should not be
#5827
Labels
Comments
This comment has been minimized.
This comment has been minimized.
3 tasks
|
Will be fixed by #5840. Since it was already a bug in 3.x I may wait a bit (~a month?) before merging the PR, as I'm worried this bugfix could otherwise be seen as a breaking change or new bug preventing migrating to the 4.0 release. |
|
This issue reproduces on master: |
|
@arcanis Do we think it is safe to move ahead with the attached PR? |
|
@arcanis, re: potential breaking changes I recently encountered a production issue where this bug allowed a build to continue even though I would consider a bugfix which causes a new version of yarn 3.x to exit with a non-zero status in this scenario to be a welcome change as this behavior is undocumented and it contravenes a user's expectations. If, upon bumping to a new version of yarn, a user encounters a change in behavior, they will learn that their project was previously built without a non-optional dependency and they can decide to modify it if this is intended. This may help people discover situations where their projects are not behaving as intended. I understand if there are other concerns which you have regarding unwelcome disruptions caused by this bugfix. |
arcanis
added a commit
that referenced
this issue
Jan 22, 2024
…#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.
merceyz
pushed a commit
that referenced
this issue
Jan 30, 2024
…#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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Self-service
Describe the bug
I am seeing a particular configuration of dependencies and package extensions where an explicit workspace dependency is considered
optional(even though it should not be consideredoptionalbecause it is an explicit workspace dependency). See the reproduction, below.In this case, the build of the explicit dependency may fail without failing the
yarn installstep, even though theyarn installstep should have failed.This behavior exists in both Yarn 3 and Yarn 4.
To reproduce
Environment
System: OS: macOS 13.6 CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Binaries: Node: 18.17.0 - /private/var/folders/82/cn2lb31s5hd572v7x5wkc1vc0000gq/T/xfs-f830d5e1/node Yarn: 4.0.0 - /private/var/folders/82/cn2lb31s5hd572v7x5wkc1vc0000gq/T/xfs-f830d5e1/yarn npm: 9.6.7 - ~/.nvm/versions/node/v18.17.0/bin/npmAdditional context
iconvas a dependency in the rootpackage.jsoncauses it to no longer reproduceThe text was updated successfully, but these errors were encountered: