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.

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

pnpm patch does not seem to work with workspaces #6048

Closed
shawnmcknight opened this issue Feb 6, 2023 · 23 comments · Fixed by #6120
Closed

pnpm patch does not seem to work with workspaces #6048

shawnmcknight opened this issue Feb 6, 2023 · 23 comments · Fixed by #6120
Assignees

Comments

@shawnmcknight
Copy link

I am attempting to patch a transitive dependency which is in the dependency tree of multiple workspaces. In this example, the dependency is extract-files@9.0.0.

From the workspace root, I run pnpm patch extract-files@9.0.0 which gives me the temporary folder link as expected. I open the folder and make the necessary changes. After completing I run pnpm patch-commit <path> which generates the patch file in the root patches folder, adds a pnpm.patchedDependencies property to the root package.json, and adds a patchedDependencies section at the top of the pnpm-lock.yaml file. However, it does not alter any of the references to that dependency elsewhere in the lockfile, and as a result, the versions being used by the workspaces are unchanged.

I revert all changes and then tried making my current working directory one of the workspace folders. I follow the same exact procedure described above and the end result is different. The patches folder is still added to the root, the root package.json still has the pnpm.patchedDependencies property, and the patchedDependencies section is added to the top of the pnpm-lock.yaml file. However, this time the lockfile has also been modified to patch the versions of that dependency relative to that workspace. My initial thought here is that the patch needs to be applied one workspace at a time.

I then change my current working directory to another workspace. I repeat the process. This time the temporary folder already shows the patch (this seems expected). I try committing the patch but there is no change. The patch was not applied to that workspace.

Basically what I'm looking to have happen here is to either apply the patch to all workspaces or at least have a procedure where I can patch each workspace one a time. Ideally something like:

pnpm -r patch dependency@version or pnpm --filter=<workspace> patch dependency@version

It's definitely possible that I'm doing something wrong here so if there is a procedure that should be taken to make this happen please let me know.

Possibly related issue: #5255

pnpm version:

7.26.3

Code to reproduce the issue:

Expected behavior:

Patches all versions of the dependency

Actual behavior:

Does not patch any versions of the dependency

Additional information:

  • node -v prints: v16.16.0
  • Windows, macOS, or Linux?: Windows
@zkochan
Copy link
Member

zkochan commented Feb 6, 2023

My initial thought here is that the patch needs to be applied one workspace at a time.

No, the patch should be applied to all projects.

@shawnmcknight
Copy link
Author

My initial thought here is that the patch needs to be applied one workspace at a time.

No, the patch should be applied to all projects.

Ok, that would definitely be appropriate in my opinion. However, when I committed the patch it did not update the lockfile for any projects and they are all still running the original unpatched version of the dependency.

I can create a reproduction if that helps.

@shawnmcknight
Copy link
Author

I've created a repository at https://github.com/shawnmcknight/pnpm-patch-tests to show a minimal reproduction.

In the repository there are two folders, each representing a pnpm root folder: root and workspaces. The root folder represents a single project configuration and the workspaces folder represents a multi-project configuration.

In the root folder, when the patch is committed, the lockfile is updated with patchedDependencies and the dependency itself is updated to point to the patched variant.

In the workspaces folder, when the patch is committed, the lockfile is only updated with patchedDependencies and the dependencies are not updated to point to the patched variant. Effectively the unpatched version of is-odd is still being used for both packages.

Please let me know if I can do anything else or answer any questions. Thanks!

@codingwithchris
Copy link

I'm having the same issue with pnpm version 7.26.3 and node version 16.18.0. .npmrc config:

save-exact=true
auto-install-peers=true
strict-peer-dependencies=true
engine-strict=true

When a package of the same version exists in multiple workspaces, the root package is being patched, but the same package in any workspace is not being patched.

@shawnmcknight
Copy link
Author

I've noticed a couple of interesting things that if I do I can get the patch commit to apply to the workspaces.

Upon running patch-commit, the lockfile is not properly updated to apply the patch to the workspace dependencies. However, if I subsequently do either of the following then the patch is applied:

  • run pnpm dedupe. Doing this modifies the lockfile to correctly apply the patch.
  • Manually remove the patchedDependencies section from the pnpm-lock.yaml file. Run pnpm install. After this, the lockfile is correctly updated to apply the patch.

@shawnmcknight
Copy link
Author

Along the same lines as the above, running pnpm install --fix-lockfile also will update the lockfile to apply the patch to the workspaces.

So it seems that running operations which don't "trust" the integrity of the lockfile (i.e. pnpm install --fix-lockfile or pnpm dedupe) or running pnpm install while the lockfile and package.json are out of sync will trigger fixing the lockfile up.

@shawnmcknight
Copy link
Author

I debugged through the workflow differences between pnpm patch-commit and pnpm install --fix-lockfile today to try to determine why fixing the lockfile makes the lockfile correct after the patch has been applied but the original patch commit did not.

From what I can tell, the primary difference between the two workflows is that pnpm install will default recursive operations and filter to all available workspaces by default due to this logic in the main cli. This ensures that when fixing the lockfile all packages are included in the evaluation. However, since the patch-commit command is not included in this condition, no packages filters are added, thus only the root package is included in the processing.

I tried changing that line from:

(cmd === 'install' || cmd === 'import' || cmd === "dedupe")

to:

(cmd === 'install' || cmd === 'import' || cmd === "dedupe" || cmd === "patch-commit")

in the hopes that this would allow things to work properly.

That change did make it so all packages were included when the patch-commit workflow called the installer. However, this introduced a different problem because the packages' manifests were read into variables at the time of filtering. Since patch-commit later modifies the root package's manifest to augment the pnpm.patchedDependencies property, the in-memory root manifest is now out of sync with the on-disk root manifest. When getOptionsFromRootManifest is called, because the in-memory manifest does not have patchedDependencies applied within it, the installer will end up doing nothing. In the existing workflow, the root manifest is read in later in installDeps because there haven't been any projects previously selected so the patchedDependencies property is available. So unfortunately my naïve approach to potentially fixing this didn't pan out.

I don't know enough about the pnpm codebase to determine how this workflow should be fixed. However, from what I can tell, the patch-commit process needs to be able to process all packages but also be aware of the patch being applied to the root manifest file. My best guess would be to add logic after the root manifest had had its pnpm.patchedDependencies property updated to execute similar filtering logic that is applied when running pnpm install to build the options for allProjects, allProjectsGraph, and selectedProjectsGraph to supply to install.

@zkochan Does this provide any additional insight into the problem? Does my proposed solution to supply project filters to install make sense? I am willing to create a PR to fix this, but I'd like to go down a path that make sense.

@await-ovo
Copy link
Member

I've created a repository at https://github.com/shawnmcknight/pnpm-patch-tests to show a minimal reproduction.

In the repository there are two folders, each representing a pnpm root folder: root and workspaces. The root folder represents a single project configuration and the workspaces folder represents a multi-project configuration.

In the root folder, when the patch is committed, the lockfile is updated with patchedDependencies and the dependency itself is updated to point to the patched variant.

In the workspaces folder, when the patch is committed, the lockfile is only updated with patchedDependencies and the dependencies are not updated to point to the patched variant. Effectively the unpatched version of is-odd is still being used for both packages.

Please let me know if I can do anything else or answer any questions. Thanks!

Hey, From the repo you provided, i just run pnpm install in workspaces directory, seems that patched version installed in packages/a and packages/b:
image

If I have misunderstood anything, please let me know, thanks a lot.

@shawnmcknight
Copy link
Author

@await-ovo Sorry, I had missed the fact that it's actually installing the dependency properly in that scenario, so maybe the setup of my reproduction isn't ideal. Seems like if node_modules isn't populated yet then things look a little better than in a normal scenario with node_modules already populated.

Locally, when I created that reproduction, after I ran pnpm patch-commit, the root package.json was updated as expected, but the pnpm-lock.yaml file was not updated properly and the dependency is not installed in to the workspace's node_modules. After running pnpm patch-commit "Already up to date" is output and the packages look like this:

image

and the lockfile looks like this, with the workspace dependencies pointing to the original is-odd@3.0.1 and not the patch hash:

image

If I re-run pnpm install while in this state, nothing changes -- the unpatched dependency is not installed.

However, if I run `pnpm install --fix-lockfile then things will change. The lockfile now looks like this, with the dependencies pointing to the patch hash:
image

and the actual dependencies are updated as well.

So I guess, unfortunately, it has to start from a blanker slate than the reproduction I provided. If you start with just workspaces a and b, no patches applied, and an up-to-date node_modules then the failure scenario occurs. I think even in the scenario you saw, where because node_modules is not up-to-date it actually installs properly is still wrong because the lockfile is incorrect as shown by pnpm install --fix-lockfile.

Let me know if I can provide any more information. Thanks!

@shawnmcknight
Copy link
Author

@await-ovo I've set up a scenario that should be easier to see the problem.
Steps:

  • Reclone the repo at https://github.com/shawnmcknight/pnpm-patch-tests. The main branch does not have the patch applied.
  • Run pnpm install to generate the unpatched node_modules
  • git checkout apply-patch. This branch has the patch applied.
  • Run pnpm install. The workspaces' node_modules will not be updated with the patch.
  • git checkout fixed-lockfile. This branch has had pnpm install --fix-lockfile run on it and the lockfile is in a valid state.
  • Run pnpm install. The workspaces' node_modules will now be updated with the patch.

The main difference between this flow and what you originally saw is that the node_modules will be populated in an unpatched state. Running pnpm install on the apply-patch branch does not properly apply the patch here because the lockfile is not in a proper state. However, once you run pnpm install on the fixed-lockfile branch everything will work properly.

@shawnmcknight
Copy link
Author

@await-ovo One more note that's probably helpful. This compare of the fixed-lockfile branch and the apply-patch branch gives a good indicator of the issue. As you can see, after fixing the lockfile, the dependencies get updated to point to the patched version rather than the original version. Its this change to the lockfile that allows pnpm install to work properly because node_modules and the lockfile are now out of sync with each other. When running pnpm install on the apply-patch branch, node_modules and the lockfile are in sync so no updates are made to node_modules.

@ChristianKaeser
Copy link

ChristianKaeser commented Feb 20, 2023

I have not followed everything in this discussion, but I have encountered the same issue that as soon as I introduce a pnpm-workspace.yaml file, any previously working patches are no longer applied.
This is reproducable with a super simple setup: https://github.com/ChristianKaeser/pnpm-patch-with-workspace-example
The two branches there show the difference in the pnpm-lock.yaml.

@await-ovo
Copy link
Member

@shawnmcknight I think I reproduced the problem and I will try to solve it

@shawnmcknight
Copy link
Author

@shawnmcknight I think I reproduced the problem and I will try to solve it

I took a peek at your PR and it looks to be what I thought would be the solution to the problem. Basically making sure that the install process is aware of the project graph before running.

Thanks for taking care of this!

@ChristianKaeser
Copy link

@zkochan @await-ovo If I understand the linked issue #6120 correctly, it should have landed in v7.28.0?

I have re-tested the simple example I linked above, and there patching still does not work with workspaces.

I have not followed everything in this discussion, but I have encountered the same issue that as soon as I introduce a pnpm-workspace.yaml file, any previously working patches are no longer applied. This is reproducable with a super simple setup: https://github.com/ChristianKaeser/pnpm-patch-with-workspace-example The two branches there show the difference in the pnpm-lock.yaml.

@shawnmcknight
Copy link
Author

@zkochan @await-ovo If I understand the linked issue #6120 correctly, it should have landed in v7.28.0?

I have re-tested the simple example I linked above, and there patching still does not work with workspaces.

I have not followed everything in this discussion, but I have encountered the same issue that as soon as I introduce a pnpm-workspace.yaml file, any previously working patches are no longer applied. This is reproducable with a super simple setup: https://github.com/ChristianKaeser/pnpm-patch-with-workspace-example The two branches there show the difference in the pnpm-lock.yaml.

Everything has seemed to work properly for me in 7.28.0. I did a patch yesterday and it all went through as expected.

@ChristianKaeser
Copy link

@zkochan @await-ovo If I understand the linked issue #6120 correctly, it should have landed in v7.28.0?
I have re-tested the simple example I linked above, and there patching still does not work with workspaces.

I have not followed everything in this discussion, but I have encountered the same issue that as soon as I introduce a pnpm-workspace.yaml file, any previously working patches are no longer applied. This is reproducable with a super simple setup: https://github.com/ChristianKaeser/pnpm-patch-with-workspace-example The two branches there show the difference in the pnpm-lock.yaml.

Everything has seemed to work properly for me in 7.28.0. I did a patch yesterday and it all went through as expected.

But you have not tried my example?

Maybe your use case's problem had a different root cause...

If so, I could also open a new ticket, if that is preferable process-wise?

@await-ovo
Copy link
Member

@ChristianKaeser Hi, you should create a package.json in workspaces root and copy pnpm.patchedDependencies settings from a/package.json to it:

// root package.json
{
  "pnpm": {
    "patchedDependencies": {
      "@low-key/vector-math@1.0.4": "patches/@low-key__vector-math@1.0.4.patch"
    }
  }
}

Then move patches directory to root directory also. After reinstalling, the patch file should work as expected.

@zkochan
Copy link
Member

zkochan commented Mar 2, 2023

We should probably print a warning when there is a patchedDependencies field not in the workspace root.

@await-ovo
Copy link
Member

We should probably print a warning when there is a patchedDependencies field not in the workspace root.

Sounds good to me ~

@ChristianKaeser
Copy link

ChristianKaeser commented Mar 3, 2023

@ChristianKaeser Hi, you should create a package.json in workspaces root and copy pnpm.patchedDependencies settings from a/package.json to it:

// root package.json
{
  "pnpm": {
    "patchedDependencies": {
      "@low-key/vector-math@1.0.4": "patches/@low-key__vector-math@1.0.4.patch"
    }
  }
}

Then move patches directory to root directory also. After reinstalling, the patch file should work as expected.

Thanks, that was the issue! I would second the wish for a warning by pnpm in this situation, especially as the docs do not mention the patching + workspace combination case last I checked. It really did not occur to me that a root package.json could be required next to the pnpm-workspace.yaml file.

@mg90707
Copy link

mg90707 commented Jun 22, 2023

Are there any plans to make local patching of a package possible without the workaround of copying it to the root package.json? Are there any reasons this is neccessary?

@zkochan
Copy link
Member

zkochan commented Jun 22, 2023

no, there are no plans. It is not a workaround. It is by design this way. Workspace projects don't have their own node_modules, all dependencies are at <workspace root>/node_modules/.pnpm, so it doesn't make sense to have patches in subdirectories.

@pnpm pnpm locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants