-
Notifications
You must be signed in to change notification settings - Fork 19
Copy PNPM patches to isolated directory #145
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
Open
0x80
wants to merge
25
commits into
main
Choose a base branch
from
thijs/1127-copy-patches-pnpm
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
c31efc2
Copy PNPM patches to isolated directory
0x80 a9e8943
Merge branch 'main' into thijs/1127-copy-patches-pnpm
0x80 1343678
1.27.0-0
0x80 3dd34f0
Use jsdoc comments consistently
0x80 1f8a8c2
Extract utility
0x80 979705d
Extract filtering logic
0x80 f704afe
Avoid patch naming collisions
0x80 0265999
Add tests for filter patched deps
0x80 b2ff870
Something
0x80 e72cae4
Use output manifest consistently
0x80 182a7cd
Add tests for copy patches
0x80 41a5251
Add tests for get package name
0x80 25fc4f2
Use correct log path function and fix formatting
0x80 f7896fe
Fix lockfile patch paths to match copied file locations
0x80 923da2e
1.27.0-1
0x80 52118ec
Format code
0x80 8b4702e
Rename misleading test for malformed scoped package
0x80 8ad26f5
Add explicit mocks for package manager and lockfile readers in tests
0x80 9b2beda
Refactor filterPatchedDependencies to use object parameter and intern…
0x80 6041e9b
Remove includePatchedDependencies config option
0x80 af08551
Simplify patch copying to preserve original folder structure
0x80 39a37aa
1.27.0-2
0x80 a94fde3
Only copy patches when output uses pnpm
0x80 c979b93
Format
0x80 ec48d5e
Update src/lib/lockfile/helpers/generate-pnpm-lockfile.ts
0x80 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,11 +13,86 @@ import { | |
| import { pruneLockfile as pruneLockfile_v8 } from "pnpm_prune_lockfile_v8"; | ||
| import { pruneLockfile as pruneLockfile_v9 } from "pnpm_prune_lockfile_v9"; | ||
| import { pick } from "remeda"; | ||
| import type { Logger } from "~/lib/logger"; | ||
| import { useLogger } from "~/lib/logger"; | ||
| import type { PackageManifest, PackagesRegistry } from "~/lib/types"; | ||
| import { getErrorMessage, isRushWorkspace } from "~/lib/utils"; | ||
| import { pnpmMapImporter } from "./pnpm-map-importer"; | ||
|
|
||
| /** | ||
| * Extracts the package name from a package spec like "[email protected]" or | ||
| * "@firebase/[email protected]" | ||
| */ | ||
| function getPackageName(packageSpec: string): string { | ||
| if (packageSpec.startsWith("@")) { | ||
| // Scoped packages: @scope/package@version -> @scope/package | ||
| const parts = packageSpec.split("@"); | ||
| return `@${parts[1] ?? ""}`; | ||
| } | ||
| // Regular packages: package@version -> package | ||
| return packageSpec.split("@")[0] ?? ""; | ||
| } | ||
|
|
||
| /** | ||
| * Filters patched dependencies to only include patches for packages that will | ||
| * actually be present in the isolated lockfile based on dependency type. | ||
| */ | ||
| function filterPatchedDependencies<T>( | ||
| patchedDependencies: Record<string, T> | undefined, | ||
| targetPackageManifest: PackageManifest, | ||
| includeDevDependencies: boolean, | ||
| log: Logger | ||
| ): Record<string, T> | undefined { | ||
| if (!patchedDependencies || typeof patchedDependencies !== "object") { | ||
| return undefined; | ||
| } | ||
|
|
||
| const filteredPatches: Record<string, T> = {}; | ||
| let includedCount = 0; | ||
| let excludedCount = 0; | ||
|
|
||
| for (const [packageSpec, patchInfo] of Object.entries(patchedDependencies)) { | ||
| const packageName = getPackageName(packageSpec); | ||
|
|
||
| // Check if it's a production dependency | ||
| if (targetPackageManifest.dependencies?.[packageName]) { | ||
| filteredPatches[packageSpec] = patchInfo; | ||
| includedCount++; | ||
| log.debug( | ||
| `Including production dependency patch in lockfile: ${packageSpec}` | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if it's a dev dependency and we should include dev dependencies | ||
| if (targetPackageManifest.devDependencies?.[packageName]) { | ||
| if (includeDevDependencies) { | ||
| filteredPatches[packageSpec] = patchInfo; | ||
| includedCount++; | ||
| log.debug(`Including dev dependency patch in lockfile: ${packageSpec}`); | ||
| } else { | ||
| excludedCount++; | ||
| log.debug( | ||
| `Excluding dev dependency patch from lockfile: ${packageSpec}` | ||
| ); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Package not found in dependencies or devDependencies | ||
| log.debug( | ||
| `Excluding patch from lockfile: ${packageSpec} (package "${packageName}" not found in target dependencies)` | ||
| ); | ||
| excludedCount++; | ||
| } | ||
|
|
||
| log.debug( | ||
| `Filtered patched dependencies: ${includedCount} included, ${excludedCount} excluded` | ||
| ); | ||
|
|
||
| return Object.keys(filteredPatches).length > 0 ? filteredPatches : undefined; | ||
| } | ||
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| export async function generatePnpmLockfile({ | ||
| workspaceRootDir, | ||
| targetPackageDir, | ||
|
|
@@ -163,13 +238,18 @@ export async function generatePnpmLockfile({ | |
| } | ||
|
|
||
| /** | ||
| * Don't know how to map the patched dependencies yet, so we just include | ||
| * them but I don't think it would work like this. The important thing for | ||
| * now is that they are omitted by default, because that is the most common | ||
| * use case. | ||
| * Filter patched dependencies to only include patches for packages that | ||
| * will actually be present in the isolated lockfile based on dependency | ||
| * type. We read patchedDependencies from workspace root lockfile, but | ||
| * filter based on target package dependencies. | ||
| */ | ||
| const patchedDependencies = includePatchedDependencies | ||
| ? lockfile.patchedDependencies | ||
| ? filterPatchedDependencies( | ||
| lockfile.patchedDependencies, | ||
| targetPackageManifest, | ||
| includeDevDependencies, | ||
| log | ||
| ) | ||
| : undefined; | ||
|
|
||
| if (useVersion9) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import fs from "fs-extra"; | ||
| import path from "node:path"; | ||
| import { useLogger } from "~/lib/logger"; | ||
| import type { PackageManifest } from "~/lib/types"; | ||
| import { getRootRelativeLogPath, readTypedJson } from "~/lib/utils"; | ||
|
|
||
| /** | ||
| * Extracts the package name from a package spec like "[email protected]" or | ||
| * "@firebase/[email protected]" | ||
| */ | ||
| function getPackageName(packageSpec: string): string { | ||
| if (packageSpec.startsWith("@")) { | ||
| // Scoped packages: @scope/package@version -> @scope/package | ||
| const parts = packageSpec.split("@"); | ||
| return `@${parts[1] ?? ""}`; | ||
| } | ||
| // Regular packages: package@version -> package | ||
| return packageSpec.split("@")[0] ?? ""; | ||
| } | ||
|
|
||
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| export async function copyPatches({ | ||
| workspaceRootDir, | ||
| targetPackageManifest, | ||
| isolateDir, | ||
| includePatchedDependencies, | ||
| includeDevDependencies, | ||
| }: { | ||
| workspaceRootDir: string; | ||
| targetPackageManifest: PackageManifest; | ||
| isolateDir: string; | ||
| includePatchedDependencies: boolean; | ||
| includeDevDependencies: boolean; | ||
| }): Promise<Record<string, string>> { | ||
| const log = useLogger(); | ||
|
|
||
| if (!includePatchedDependencies) { | ||
| log.debug("Skipping patch copying (includePatchedDependencies is false)"); | ||
| return {}; | ||
| } | ||
|
|
||
| let workspaceRootManifest: PackageManifest; | ||
| try { | ||
| workspaceRootManifest = await readTypedJson<PackageManifest>( | ||
| path.join(workspaceRootDir, "package.json") | ||
| ); | ||
| } catch (error) { | ||
| log.warn( | ||
| `Could not read workspace root package.json: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| return {}; | ||
| } | ||
|
|
||
| const patchedDependencies = workspaceRootManifest.pnpm?.patchedDependencies; | ||
|
|
||
| if (!patchedDependencies || Object.keys(patchedDependencies).length === 0) { | ||
| log.debug("No patched dependencies found in workspace root package.json"); | ||
| return {}; | ||
| } | ||
|
|
||
| const patchesDir = path.join(isolateDir, "patches"); | ||
| await fs.ensureDir(patchesDir); | ||
|
|
||
| log.debug( | ||
| `Found ${Object.keys(patchedDependencies).length} patched dependencies in workspace` | ||
| ); | ||
|
|
||
| const filteredPatches = Object.entries(patchedDependencies).filter( | ||
| ([packageSpec]) => { | ||
| const packageName = getPackageName(packageSpec); | ||
|
|
||
| // Check if it's a production dependency | ||
| if (targetPackageManifest.dependencies?.[packageName]) { | ||
| log.debug(`Including production dependency patch: ${packageSpec}`); | ||
| return true; | ||
| } | ||
|
|
||
| // Check if it's a dev dependency and we should include dev dependencies | ||
| if (targetPackageManifest.devDependencies?.[packageName]) { | ||
| if (includeDevDependencies) { | ||
| log.debug(`Including dev dependency patch: ${packageSpec}`); | ||
| return true; | ||
| } | ||
| log.debug( | ||
| `Excluding dev dependency patch: ${packageSpec} (includeDevDependencies=false)` | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| log.debug( | ||
| `Excluding patch ${packageSpec}: package "${packageName}" not found in target dependencies` | ||
| ); | ||
| return false; | ||
| } | ||
| ); | ||
|
|
||
| log.debug( | ||
| `Copying ${filteredPatches.length} patches (filtered from ${Object.keys(patchedDependencies).length})` | ||
| ); | ||
|
|
||
| const copiedPatches: Record<string, string> = {}; | ||
|
|
||
| for (const [packageSpec, patchPath] of filteredPatches) { | ||
| const sourcePatchPath = path.resolve(workspaceRootDir, patchPath); | ||
| const targetPatchPath = path.join(patchesDir, path.basename(patchPath)); | ||
|
|
||
| if (!fs.existsSync(sourcePatchPath)) { | ||
| log.warn( | ||
| `Patch file not found: ${getRootRelativeLogPath(sourcePatchPath, workspaceRootDir)}` | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| await fs.copy(sourcePatchPath, targetPatchPath); | ||
| log.debug(`Copied patch for ${packageSpec}: ${path.basename(patchPath)}`); | ||
|
|
||
| copiedPatches[packageSpec] = `patches/${path.basename(patchPath)}`; | ||
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| if (Object.keys(copiedPatches).length > 0) { | ||
| log.debug( | ||
| `Patches copied to ${getRootRelativeLogPath(patchesDir, isolateDir)}` | ||
0x80 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ); | ||
| } | ||
|
|
||
| return copiedPatches; | ||
| } | ||
0x80 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.