copy patches with pnpm #142

kevpie · 2025-09-25T06:52:12Z

Copies the relevant patches to the isolate folder along with package.json section.

Probably a better way to do this.
For now I force firebase-tools-with-isolate to provide the right config using a patch like this.

diff --git a/lib/deploy/functions/prepareFunctionsUpload.js b/lib/deploy/functions/prepareFunctionsUpload.js
index f6cf911cd7d67983e57b50aa05a8289d4da6484b..b417d22524588ffb93fe221dae49590de9b762b4 100644
--- a/lib/deploy/functions/prepareFunctionsUpload.js
+++ b/lib/deploy/functions/prepareFunctionsUpload.js
@@ -99,9 +99,12 @@ async function runIsolate(sourceDirName) {
     try {
         utils.logLabeledBullet("isolate", `Isolating the source`);
         const { isolate } = (await (0, dynamicImport_1.dynamicImport)("isolate-package"));
+        console.log("isolateDir!!", sourceDirName);
         const isolateDir = await isolate(sourceDirName !== "."
             ? {
                 targetPackagePath: path.join("./", sourceDirName),
+                includePatchedDependencies: true,
+                workspaceRoot: "..",
             }
             : undefined);
         utils.logLabeledBullet("isolate", `Finished isolation at ${clc.bold(isolateDir)}`);

- Add @esbuild-plugins/tsconfig-paths to handle ~/lib/* imports - Use createRequire to import CommonJS plugin in ESM context - Resolves build failures in CI environments where baseUrl context differs

kevpie · 2025-09-26T01:29:05Z

tsup.config.ts

  clean: true,
+  esbuildPlugins: [tsconfigPathsPlugin({})],
  // shims: true, // replaces use of import.meta
  /**


Building inside a pnpm install that has a newer version of esbuild. Esbuild struggles with the paths without the plugin to hoist in the paths from tsconfig.

jieey1140 · 2025-11-06T09:11:36Z

I wish this feature would be released soon!

Copilot

Pull request overview

This PR adds support for copying pnpm patches to the isolated package folder when includePatchedDependencies is enabled. The implementation filters patches based on whether the target package has dependencies on the patched packages, respecting both production and dev dependency types.

Key changes:

Introduces patch file copying logic that filters patches based on target package dependencies
Updates lockfile generation to filter patchedDependencies in the same manner
Extends the PackageManifest type to include pnpm-specific fields

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tsup.config.ts	Adds esbuild plugin for TypeScript path resolution
src/lib/types.ts	Extends PackageManifest type to include pnpm.patchedDependencies field
src/lib/patches/copy-patches.ts	New module implementing patch file copying with dependency-based filtering
src/lib/lockfile/helpers/generate-pnpm-lockfile.ts	Adds filtering logic for patchedDependencies in lockfiles and PNPM 10+ support
src/isolate.ts	Integrates patch copying into the isolation workflow
package.json	Adds @esbuild-plugins/tsconfig-paths dev dependency
pnpm-lock.yaml	Updates lockfile with new dependency and consolidated debug version

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T19:35:37Z

src/lib/patches/copy-patches.ts

+  const getPackageName = (packageSpec: string): string => {
+    // Handle scoped packages: @scope/package@version -> @scope/package
+    if (packageSpec.startsWith("@")) {
+      const parts = packageSpec.split("@");
+      return `@${parts[1]}`;
+    }
+    // Handle regular packages: package@version -> package
+    return packageSpec.split("@")[0];
+  };


The getPackageName function is duplicated in src/lib/lockfile/helpers/generate-pnpm-lockfile.ts (lines 35-43). Extract this utility function to a shared location (e.g., src/lib/utils/get-package-name.ts) to avoid code duplication and ensure consistent behavior across both modules.

Copilot · 2025-11-27T19:35:38Z

src/lib/lockfile/helpers/generate-pnpm-lockfile.ts

 import { pnpmMapImporter } from "./pnpm-map-importer";

+function filterPatchedDependencies(
+  originalPatchedDependencies: any,


The any type should be replaced with a more specific type. Consider using Record<string, string> or defining a proper type for patchedDependencies based on the pnpm specification.

Copilot · 2025-11-27T19:35:38Z

src/lib/lockfile/helpers/generate-pnpm-lockfile.ts

+  targetPackageManifest: PackageManifest,
+  includeDevDependencies: boolean,
+  log: Logger
+): any {


The return type any should be replaced with a more specific type such as Record<string, string> | undefined to improve type safety.

Copilot · 2025-11-27T19:35:38Z

src/lib/lockfile/helpers/generate-pnpm-lockfile.ts

+    return packageSpec.split("@")[0];
+  };
+
+  const filteredPatches: any = {};


Replace any type with Record<string, string> or the appropriate type for patchedDependencies to improve type safety.

Copilot · 2025-11-27T19:35:39Z

src/lib/patches/copy-patches.ts

+export async function copyPatches({
+  workspaceRootDir,
+  targetPackageManifest,
+  isolateDir,
+  includePatchedDependencies,
+  includeDevDependencies,
+}: {
+  workspaceRootDir: string;
+  targetPackageManifest: PackageManifest;
+  isolateDir: string;
+  includePatchedDependencies: boolean;
+  includeDevDependencies: boolean;
+}): Promise<Record<string, string>> {


The new copyPatches function lacks test coverage. Consider adding tests to verify: (1) correct filtering of patches based on dependency types, (2) handling of scoped vs non-scoped packages, (3) behavior when includePatchedDependencies is false, (4) error handling when workspace root package.json cannot be read, and (5) proper file copying to the isolate directory.

Copilot · 2025-11-27T19:35:39Z

src/lib/lockfile/helpers/generate-pnpm-lockfile.ts

+function filterPatchedDependencies(
+  originalPatchedDependencies: any,
+  targetPackageManifest: PackageManifest,
+  includeDevDependencies: boolean,
+  log: Logger
+): any {


The new filterPatchedDependencies function lacks test coverage. Consider adding tests to verify the filtering logic for production dependencies, dev dependencies, and packages not found in target dependencies.

0x80 · 2025-11-27T20:36:23Z

@kevpie @jieey1140 I have upgraded to more modern tooling and moved away from tsup, and there were a few things in here that I didn't want to merge as-is, so I found it easier to let Opus create a new PR based on this.

Please review #145 and see if I missed anything. I don't have projects where I need this, and I don't have proper tests for this, so please try out the prerelease 1.27.0-0 under tag "next" and let me know if it works for you.

Also, I wonder about the setting needed to enable this behavior. Don't you think we can assume that if you define patched dependencies in your manifest file, that you'd always want to copy them?

0x80 · 2025-11-27T20:39:29Z

Closed in favor of #145

kevpie · 2025-11-27T20:50:47Z

Thank you @0x80! I'll try out the new PR soon using tsdown.

0x80 · 2025-11-27T21:19:19Z

@kevpie @jieey1140 Cursor and Copilot came with a lot of seemingly good suggestions and I adopted them, but it was a bit of a snowball effect, and I don't have time to go over things carefully.

I've created a new version 1.27.0-1 with all the changes that are listed in the PR description under subsequent changes.

I hope it still works with all of that 🤞

0x80 · 2025-11-27T21:50:12Z

Ok it seems AI was a bit silly flattening the copied patches and then attempting to avoid the naming collisions that could be caused by that. I will revert that and preserve the original folder structure of the patches.

So 1.27.0-2 coming up...

copy patches

02f5cbb

kevpie mentioned this pull request Sep 25, 2025

PNPM Patches 0x80/firebase-tools-with-isolate#23

Open

kevpie added 3 commits September 25, 2025 00:03

Merge branch 'main' into pnpm-with-patches

9ac4c32

Fix TypeScript path resolution for pnpm store builds

d7d3f5c

- Add @esbuild-plugins/tsconfig-paths to handle ~/lib/* imports - Use createRequire to import CommonJS plugin in ESM context - Resolves build failures in CI environments where baseUrl context differs

Remove vim swap file

a90acc0

kevpie commented Sep 26, 2025

View reviewed changes

0x80 requested a review from Copilot November 27, 2025 19:32

Copilot started reviewing on behalf of 0x80 November 27, 2025 19:33 View session

Copilot finished reviewing on behalf of 0x80 November 27, 2025 19:34

Copilot AI reviewed Nov 27, 2025

View reviewed changes

0x80 mentioned this pull request Nov 27, 2025

Copy PNPM patches to isolated directory #145

Open

0x80 closed this Nov 27, 2025

copy patches with pnpm #142

copy patches with pnpm #142

Uh oh!

Conversation

kevpie commented Sep 25, 2025

Uh oh!

kevpie Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

jieey1140 commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

0x80 commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x80 commented Nov 27, 2025

Uh oh!

kevpie commented Nov 27, 2025

Uh oh!

0x80 commented Nov 27, 2025

Uh oh!

0x80 commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0x80 commented Nov 27, 2025 •

edited

Loading