-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(why): allow specifying a version or range #6992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(why): allow specifying a version or range #6992
Conversation
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
|
@arcanis are you available to review this? You refactored |
| `Explain why version 3.3.1 of lodash is in your project`, | ||
| `$0 why lodash@3.3.1`, | ||
| ], [ | ||
| `or why version 3.X of lodash is in your project`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `or why version 3.X of lodash is in your project`, | |
| `Explain why version 3.x of lodash is in your project`, |
| function isSameIdent(pkg: Ident, targetPkg: Ident) { | ||
| return pkg.identHash === targetPkg.identHash; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function already exists as structUtils.areIdentsEqual. But honestly, I think just inlining this is much more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting. I replaced them all with structUtils.areIdentsEqual. It reads more like a natural language which, for me at least, increases readability.
| if (!isSameIdent(nextPkg, targetPkg)) | ||
| continue; | ||
|
|
||
| if (!structUtils.isPackageInRange(nextPkg, targetPkg.range)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works for semver ranges, right? What happens if the given descriptor's range is non-semver? Either way, I think that should fail-fast above, when parsing the descriptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a case I didn't consider. I added a new test case for this
| seen.add(pkg.locatorHash); | ||
|
|
||
| if (pkg.identHash === identHash) { | ||
| if (isSameIdent(pkg, targetPkg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just can in-range-ness here and eliminate the checks below? Or is there an edge case I am not considering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Changed it to your suggestion
…w-version-range-for-yarn-why
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
Signed-off-by: Remy Parzinski <remypar5@users.noreply.github.com>
| // We don't want to print the full path if it doesn't transitively depend on targetPkg.range | ||
| if (structUtils.areIdentsEqual(pkg, targetPkg) && !structUtils.isPackageInRange(pkg, targetPkg.range)) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can also be removed? dependents already only contains packages that transitively depend on the specified name and range. Again, not sure if I'm missing an edge case
What's the problem this PR addresses?
Allowing users to specify a version or range for
yarn whyto find out why a specific version appears in the dependency tree. This is particularly useful if you want to address CVEs.Closes #6859
How did you fix it?
Checklist