Fix: Don't do range collapsing when using a frozen lockfile #3604
Conversation
src/cli/commands/import.js
Outdated
| @@ -280,7 +280,7 @@ export class Import extends Install { | |||
| if (manifest.name && this.resolver instanceof ImportPackageResolver) { | |||
| this.resolver.rootName = manifest.name; | |||
| } | |||
| await this.resolver.init(requests, this.flags.flat); | |||
| await this.resolver.init(requests, this.flags.flat, this.flags.frozenLockfile); | |||
There was a problem hiding this comment.
Could we change this to named args, like this:
this.resolver.init(requests, {
flat: this.flags.flat,
frozenLockFile: this.flags.frozenLockfile,
});
?
There was a problem hiding this comment.
Dang, this time I've been 28 seconds slower 😆
There was a problem hiding this comment.
Damn! I actually did it that way first and then went back to this monstrosity :(
There was a problem hiding this comment.
Good for merge! 😃 However, note that we're trying to get away from boolean traps, so in the future this kind of flag should be passed in a trailing object 👍
|
@arcanis - Please do not merge until I defuse the boolean trap ;) |
src/cli/commands/import.js
Outdated
| async init(deps: DependencyRequestPatterns, isFlat: boolean): Promise<void> { | ||
| async init( | ||
| deps: DependencyRequestPatterns, | ||
| {isFlat, isFrozen}: {isFlat: boolean, isFrozen?: boolean}, |
There was a problem hiding this comment.
Shouldn't isFlat be also a isFlat? in its type? I'm not really familiar with Flow, though. On a different note, you can add a default value ( = {} at the end of the parameter declaration) so that you don't have to explicitely pass an empty object (like here). That might require to move the workspaceLayout parameter before the options.
There was a problem hiding this comment.
Nice! I thought about default params but wasn't sure if we supported them. Since now I know, I'll go ahead and use it.
src/package-resolver.js
Outdated
| @@ -15,6 +15,12 @@ import WorkspaceLayout from './workspace-layout.js'; | |||
| const invariant = require('invariant'); | |||
| const semver = require('semver'); | |||
|
|
|||
| export type ResolverOptions = { | |||
There was a problem hiding this comment.
Can you make it an exact type using:
export type ResolverOptions = {|
…
|}
that way you cannot add fields to the object that aren't specified here. In general, I recommend all object type definitions to use this unless they have a good reason not to.
Summary
Fixes #3313.
Yarn automatically optimizes less-than-ideal
yarn.lockfiles, usually from older versions. That said when run with the--frozen-lockfileargument, it should neither touch the lockfile nor throw an exception if the lockfile satisfies all the needs, even if it can be optimized.Test plan
Existing unit tests plus a new unit test which fails without the fix applied.