Variants #1
Variants #1
Conversation
| // Use the VariantRemapResolver | ||
| const remapDescriptor = structUtils.makeDescriptor( | ||
| parentDependencyPkgDescriptor, | ||
| `variant:${structUtils.stringifyDescriptor(boundMatchDescriptor)}` |
There was a problem hiding this comment.
If I build this with the structUtils helper, it html encodes characters that mean the resolver can't grab the matched descriptor.
There was a problem hiding this comment.
npm:[email protected] was being converted to npm%[email protected]
| const pkgWorkspace = workspace.project.tryWorkspaceByLocator(pkg); | ||
|
|
||
| // The package has to have a version | ||
| let pkgVersion = pkg.version; |
There was a problem hiding this comment.
Do packages at this stage always have a version?
There was a problem hiding this comment.
Yes, even link: packages have their version set to 0.0.0
There was a problem hiding this comment.
Left a few comments. So far the main difficulty I see is in the concept of parameter inheritance; currently the resolveEverything function is kinda meant to make a simple range -> package resolution, without any idea of the tree shape itself. Adding a hierarchical traversal into it is a little difficult and will come with edge cases.
| { | ||
| "name": "variant-fallback", | ||
| "version": "0.12.22" | ||
| } |
There was a problem hiding this comment.
I think you accidentally put this package in the wrong folder (it should be in the acceptance tests folder)
There was a problem hiding this comment.
I was testing just 'in the yarn repo' since the iteration speed was faster, but I'll clean this up before the actual PR.
| platform: process.platform, | ||
| arch: process.arch, | ||
| abi: process.versions.modules, |
There was a problem hiding this comment.
@lovell post contains a few suggestions: yarnpkg#2751 (comment)
There was a problem hiding this comment.
Would we want to include something like detect-libc?
| }; | ||
|
|
||
| if ((process.versions as any).napi) | ||
| parameterComparators.napi = (parameterValue, possiblityValue) => parseInt(parameterValue) >= parseInt(possiblityValue); |
There was a problem hiding this comment.
Perhaps we should have a variantUtils file that would expose predefined variant "types". So that for example:
parameterComparators.napi = variantUtils.makeIntParameter();Would also make it easier if we want to add some validation sometime.
| return variantParameters; | ||
| }, | ||
|
|
||
| reduceVariantStartingParameters: (variantParameters: VariantParameters, project: Project, workspace: Workspace) => { |
There was a problem hiding this comment.
Note: we only use reducer hooks when we expect users to return a completely different object (for example in wrapScriptExecution we completely replace the script execution function). For this particular case, I think it's better to use a classic trigger hook, where users will be expected to simply mutate variantParameters (same thing for the other hooks).
| if (Array.isArray(data.variants)) { | ||
| this.variants = data.variants; | ||
| } else { | ||
| this.variants = [data.variants]; // TODO: Actually parse this instead of blindly copying |
There was a problem hiding this comment.
Perhaps we could use typanion, since it's already there for our CLI options?
| // If the package is a workspace, access the actual workspace to get the actual version in case it's 0.0.0-use-local | ||
| if (pkgWorkspace && pkgWorkspace.manifest.version) | ||
| pkgVersion = pkgWorkspace.manifest.version; |
There was a problem hiding this comment.
I don't think that's necessary; 0.0.0-use.local is only set inside the lockfile, but never stored anywhere after reading it.
| const possibilities = combineVariantMatrix(matrix, potentialVariants.exclude); | ||
| possibilities.push(...(potentialVariants.include ?? [])); |
There was a problem hiding this comment.
I wonder if for performance purposes it wouldn't be a good idea to rather:
- check whether the current parameter set matches the includes list
- check whether the current parameter set matches non of the excludes list
- check that all the current parameters are valid per the matrix
In other words, rather than computing the full set of matrix combinations then finding whether one matches the current parameters, do the other way around: check that the parameters match the matrix requirements.
This would also avoid the combinatorial explosion problem.
There was a problem hiding this comment.
Good idea. We'll still need to generate the combinatoral set for the cache, but for this yeah that should be faster.
| if (!alreadyResolved) { | ||
| opts.report.reportInfo(MessageName.UNNAMED, `Adding variant cache entry:, ${ | ||
| structUtils.prettyLocator(this.configuration, pkg) | ||
| } -> ${ | ||
| structUtils.prettyLocator(this.configuration, cacheEntry) | ||
| }`); | ||
| } |
There was a problem hiding this comment.
Note that I'm not sure we should print those messages - the resolutions messages are important, and keeping it trimmed to the essential is important to avoid people skipping important warnings. Plus it seems it would simplify the code a little bit.
There was a problem hiding this comment.
They're more for debugging right now, I defer to you on what we should print in the actual PR.
| const resolveAttempt = await scheduleDescriptorResolution(remapDescriptor, variantParameters, workspace, pkgParent); | ||
|
|
||
| // Remap the dependency to the remapDescriptor if it succeeded | ||
| pkgParentOrWorkspace.dependencies.set(parentDependencyPkgIdentHash, remapDescriptor); |
There was a problem hiding this comment.
Mutating the parent is problematic, because it's shared between all the dependency branches. For instance let's say you have two workspaces foo and bar that both depend on pkg which itself depends on lib, which has variants. Now let's say that both foo and bar provide different parameter sets - then we'll need pkg to depend on different versions of lib, but at the moment that's not possible since pkgParentOrWorkspace will be the same value in both cases, so they'll override each other 🤔
That being said I don't have an immediate solution in mind, but this is a significant issue. Perhaps parameters should have the same restriction as peer dependencies and have to be declared at every level, even if only to explicitly "inherit" a parameter. This way we'd be able to virtualize pkg by seeing that it defines a parameter set.
In this case however, the variants would only be resolved inside applyVirtualResolutionMutations (with the rest of the resolution pipeline being reserved to just adding the package that we're going to need into the resolution store).
There was a problem hiding this comment.
Ah you're right. The explicit inheritance would work but that API feels a bit verbose. I'll have a think about it though.
|
|
||
| allPackages.set(pkg.locatorHash, pkg); | ||
|
|
||
| return pkg; | ||
| }; | ||
|
|
||
| const schedulePackageResolution = async (locator: Locator) => { | ||
| const schedulePackageResolution = async (locator: Locator, variantParameters: VariantParameters, workspace: Workspace, pkgParent?: Package) => { |
There was a problem hiding this comment.
One problem with schedulePackageResolution is that it'll cache the variant resolutions. I saw you attempted a guard against that by removing the package entry from the cache when it has a variant spec, but it won't be enough in the current design: to reuse the previous example, even if you evict lib from the cache, pkg will be cached too (since it doesn't have variants) and thus won't be traversed twice, causing lib to only be traversed once as well.
This could be fixed by enforcing explicit parameters (like I suggested before) and evicting pkg once we see it declares parameters (suggesting it will define variants down the road that will require multiple resolutions).
Draft PR for the Variant Package RFC.