Yarn2: Improve dependency handling#11972
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11972 +/- ##
============================================
+ Coverage 58.44% 58.58% +0.13%
- Complexity 1811 1819 +8
============================================
Files 361 361
Lines 13504 13562 +58
Branches 1385 1404 +19
============================================
+ Hits 7893 7945 +52
- Misses 5115 5116 +1
- Partials 496 501 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| import java.io.File | ||
|
|
||
| import org.apache.logging.log4j.kotlin.logger |
There was a problem hiding this comment.
When packages with different versions appear in the dependency graph, Yarn resolves a specific version.
Let me ensure I understand this sentence correctly. Are you saying, if a project transitively depends on package "foo" both in version "1.0.0" and "2.0.0", then only either version will be resolved?
If so, that would contradict my understand of the Node ecosystem's capability to indeed support multiple versions of the same package in the transitive tree.
There was a problem hiding this comment.
I have rephrased the commit message to hopefully make it clearer. The problem with the current implementation is that it looks up PackageInfo objects using a key which contains a specific version. In this project, however, there are references to dependencies with slightly different versions.
It has been observed that packages in the dependency graph reference other packages with a different version than was actually installed. The current implementation could not handle this correctly, but failed to look up the `PackageInfo` for affected packages. Fix this by implementing fallback logic for virtual packages and packages with changed versions. Signed-off-by: Oliver Heger <oliver.heger@bosch.com>
Handle the case that a package appears multiple times with different versions in the dependency graph. In this case, try to select the correct version based on semantic version ranges. Signed-off-by: Oliver Heger <oliver.heger@bosch.com>
oheger-bosch
force-pushed
the
oheger-bosch/yarn2_dependencies
branch
from
32e3553 to
81d9b7c
Compare
|
|
||
| override fun dependenciesFor(dependency: PackageInfo): List<PackageInfo> = | ||
| dependency.children.dependencies.map { packageInfoForLocator.getValue(it.realLocator) } | ||
| dependency.children.dependencies.map(this::packageInfoFor) |
| } | ||
| }) | ||
|
|
||
| private val WORKING_DIR = File(".") |
There was a problem hiding this comment.
Wouldn't it be safer / better practice to use a temporary directory here?
| return when (candidates.size) { | ||
| 1 -> candidates.single().also { | ||
| logger.debug { | ||
| "Resolved locator '${dependency.realLocator}' to '${it.value}' via module name lookup." | ||
| } | ||
| } | ||
|
|
||
| 0 -> error( | ||
| "Could not find a PackageInfo for locator '${dependency.realLocator}'. No entry for module " + | ||
| "'$moduleName' exists in ${packageInfoForLocator.keys}." | ||
| ) | ||
|
|
||
| else -> error( | ||
| "Could not unambiguously resolve locator '${dependency.realLocator}'. Found ${candidates.size} " + | ||
| "installed versions of module '$moduleName': ${candidates.map { it.value }}." | ||
| ) | ||
| } |
There was a problem hiding this comment.
Rewriting this as
candidates.singleOrNull()?.also {
logger.debug {
"Resolved locator '${dependency.realLocator}' to '${it.value}' via module name lookup."
}
return it
}
if (candidates.isEmpty()) {
error(
"Could not find a PackageInfo for locator '${dependency.realLocator}'. No entry for module " +
"'$moduleName' exists in ${packageInfoForLocator.keys}."
)
}
error("Could not unambiguously resolve locator '${dependency.realLocator}'. Found ${candidates.size} " +
"installed versions of module '$moduleName': ${candidates.map { it.value }}.")probably is a bit more readable, and keep the diff to the next commit smaller.
| if (descriptorRemainder.startsWith("npm:")) { | ||
| val rangeSpec = descriptorRemainder.removePrefix("npm:") |
There was a problem hiding this comment.
Prefer using withoutPrefix().
| val range = runCatching { RangeListFactory.create(rangeSpec) }.getOrNull() | ||
| if (range != null) { |
There was a problem hiding this comment.
Prefer
runCatching {
RangeListFactory.create(rangeSpec)
}.onSuccess { range ->
| if (matchingCandidates.size == 1) { | ||
| return matchingCandidates.single().also { |
There was a problem hiding this comment.
Prefer using singleOrNull() to not both check the size and eventually return the single value.
2 participants
This PR addresses issues in the Yarn2+ implementation that were discovered in a project where dependencies with multiple versions were referenced. In this case, Yarn resolves a dependency version which might not match the one contained in locators, and thus the lookup for
PackageInfoobjects fails. With the changes provided here, such issues could be fixed.