Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: --lockfile-only and readPackage hook modifying workspace packages #5678

Merged
merged 3 commits into from Nov 26, 2022
Merged

fix: --lockfile-only and readPackage hook modifying workspace packages #5678

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b6a81
fix: `--lockfile-only` and `readPackage` hook modifying workspace pac…
Silic0nS0ldier Nov 24, 2022
e7607d1
test: use `unlink` over `rm` to support NodeJS 14
Silic0nS0ldier Nov 24, 2022
d83e959
docs: add changesets
zkochan Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg-manager/core/src/install/index.ts
View file
Open in desktop
Expand Up @@ -690,6 +690,7 @@ export type ImporterToUpdate = {
} & DependenciesMutation

export interface UpdatedProject {
originalManifest?: ProjectManifest
manifest: ProjectManifest
peerDependencyIssues?: PeerDependencyIssues
rootDir: string
Expand Down
2 changes: 1 addition & 1 deletion pkg-manager/plugin-commands-installation/src/recursive.ts
View file
Open in desktop
Expand Up @@ -257,7 +257,7 @@ export async function recursive (
if (opts.save !== false) {
await Promise.all(
mutatedPkgs
.map(async ({ manifest }, index) => writeProjectManifests[index](manifest))
.map(async ({ originalManifest, manifest }, index) => writeProjectManifests[index](originalManifest ?? manifest))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem right to me.

First of all, why do you write the original manifest at all? If it is the original one, then it is already the one that is saved in the file system.

Secondly, this means that the file will not be updated when opts.save is true. It doesn't make sense.

Maybe what you need to do is changing line 257 to:

if (opts.save !== false && !opts.lockfileOnly) {

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposed fix may well be wrong, but I'll add some context around how I got here.

Elsewhere in the repo I noticed .manifest and .originalManifest are both modified to apply changes like removing dependencies and something in @pnpm/resolve-dependencies that I assume handles adding and upgrading.

  • Removing, replaces manifest object properties.

    pnpm/pkg-manager/core/src/install/index.ts

    Lines 749 to 752 in 54128c9

    project.manifest = await _removeDeps(project.manifest)
    if (project.originalManifest != null) {
    project.originalManifest = await _removeDeps(project.originalManifest)
    }
  • Adding and upgrading (I think), mutates input manifest objects.

    pnpm/pkg-manager/resolve-dependencies/src/updateProjectManifest.ts

    Lines 45 to 57 in 54128c9

    const hookedManifest = await updateProjectManifestObject(
    importer.rootDir,
    importer.manifest,
    specsToUpsert
    )
    const originalManifest = (importer.originalManifest != null)
    ? await updateProjectManifestObject(
    importer.rootDir,
    importer.originalManifest,
    specsToUpsert
    )
    : undefined
    return [hookedManifest, originalManifest]

Interpretation as such was "originalManifest must mean the manifest without mutations from the readPackage hook".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original manifest is modified, sure. If I remember correctly, original manifest is only not modified by changes done by the hooks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, you might be right. This is what you had to change. The names of variables are confusing.

)
}
return true
Expand Down
25 changes: 25 additions & 0 deletions pnpm/test/hooks.ts
View file
Open in desktop
Expand Up @@ -37,6 +37,18 @@ test('readPackage hook in single project doesn\'t modify manifest', async () =>
pkg = await loadJsonFile(path.resolve('package.json'))
expect(pkg.dependencies).toBeFalsy() // remove & readPackage hook work
await project.hasNot('is-positive')

// Reset for --lockfile-only checks
await fs.unlink('pnpm-lock.yaml');

await execPnpm(['install', '--lockfile-only'])
pkg = await loadJsonFile(path.resolve('package.json'))
expect(pkg.dependencies).toBeFalsy() // install --lockfile-only & readPackage hook work, without pnpm-lock.yaml

// runs with pnpm-lock.yaml should not mutate local projects
await execPnpm(['install', '--lockfile-only'])
pkg = await loadJsonFile(path.resolve('package.json'))
expect(pkg.dependencies).toBeFalsy() // install --lockfile-only & readPackage hook work, with pnpm-lock.yaml
})

test('readPackage hook in monorepo doesn\'t modify manifest', async () => {
Expand Down Expand Up @@ -79,6 +91,19 @@ test('readPackage hook in monorepo doesn\'t modify manifest', async () => {
await execPnpm(['remove', 'is-positive', '--filter', 'project-a'])
pkg = await loadJsonFile(path.resolve('project-a/package.json'))
expect(pkg.dependencies).toBeFalsy() // remove & readPackage hook work

// Reset for --lockfile-only checks
await fs.unlink('pnpm-lock.yaml');

await execPnpm(['install', '--lockfile-only'])
pkg = await loadJsonFile(path.resolve('project-a/package.json'))
expect(pkg.dependencies).toBeFalsy() // install --lockfile-only & readPackage hook work, without pnpm-lock.yaml

// runs with pnpm-lock.yaml should not mutate local projects
await execPnpm(['install', '--lockfile-only'])
pkg = await loadJsonFile(path.resolve('project-a/package.json'))
expect(pkg.dependencies).toBeFalsy() // install --lockfile-only & readPackage hook work, with pnpm-lock.yaml

})

test('filterLog hook filters peer dependency warning', async () => {
Expand Down