adding resolutions should cause lockfile regeneration #5603
Conversation
| async (config, reporter): Promise<void> => { | ||
| const packageJson = await fs.readFile(path.join(config.cwd, 'package.json')); | ||
| // create new package.json with resolutions which override e/left-pad version | ||
| const newPackageJson = packageJson.replace(/}\n}/g, ' },\n"resolutions": {\n "e/left-pad": "1.1.1"\n }\n}'); |
There was a problem hiding this comment.
Can you use JSON.parse, modify the parsed object and then use JSON.stringify to write it back instead of using .replace on the JSON string?
| const reinstall = new Install({}, config, reporter, await Lockfile.fromDirectory(config.cwd)); | ||
| await reinstall.init(); | ||
| // don't expect left-pad in e/node_modules since it's now being replaced by single dependency [email protected] | ||
| expect(await fs.exists(path.join(config.cwd, 'node_modules', 'e', 'node_modules', 'left-pad', 'index.js'))).toBe( |
There was a problem hiding this comment.
I'd also verify that the lockfile got updated here.
|
tests failed because of the new version of left-pad, so snapshot doesn't match |
| > => { | ||
| return runInstall( | ||
| {}, | ||
| {source: 'resolutions', cwd: 'adding-resolutions-should-casue-lockfile-regeneration'}, |
| // write new package.json | ||
| await fs.writeFile(path.join(config.cwd, 'package.json'), JSON.stringify(newPackageJson)); | ||
| // run install again | ||
| const reinstall = new Install({}, config, reporter, await Lockfile.fromDirectory(config.cwd)); |
There was a problem hiding this comment.
can you add an assertion before the reinstall the reverse of this logic (it should have a nested node module following the correct semver version?)
src/package-resolver.js
Outdated
| getPackageRemoteResolved(manifest: Manifest): ?string { | ||
| const packageReference: ?PackageReference = manifest._reference; | ||
| const packageRemote: ?PackageRemote = packageReference ? packageReference.remote : null; | ||
| return packageRemote ? packageRemote.resolved : null; |
There was a problem hiding this comment.
I don't think you need to do all the null checks given the types and invariant check... Also I'm seeing that this can be more of a helper/validation logic with what you have below that could later be moved to integrity checker, i.e. if there is a lockfile version, does the lockfile version match the resolution's resolved version?
// in resolution-map.js
export const shouldUpdateLockfile = (lockfileEntry: ?LockManifest, resolutionEntry: ?PackageReference) => {
if (!lockfileEntry || !resolutionEntry) {
return false;
}
return lockfileEntry.resolved !== resolutionEntry.remote.resolved;
}
// then call below;
if (shouldUpdateLockfile(lockManifest, resolutionManifest._reference)) {
this.lockfile.removePattern(pattern);
}
maybe something like this would work? wdyt?
There was a problem hiding this comment.
sure, this looks more readable
Summary
After PR #5572 if you already have package.json with some dependencies and generated lockfile, after adding resolution and running
yarn installlockfile wont be updated and package overriden by resolution will not be removedTest plan
added new test which checks that after adding resolution, redundant package removed, maybe it is better to check the lockfile itself?
before:
after:
