Skip to content
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"comment": "In PnpmShrinkwrapFile getIntegrityForImporter, remove the external filter and include workspace-local link: dependencies by recursing into their importer entries, so shrinkwrap-deps.json hashes cover the full dependency tree.",
"type": "patch",
"packageName": "@microsoft/rush"
}
],
"packageName": "@microsoft/rush"
}
47 changes: 32 additions & 15 deletions libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -923,24 +923,46 @@ export class PnpmShrinkwrapFile extends BaseShrinkwrapFile {

const { dependencies, devDependencies, optionalDependencies } = importer;

const externalFilter: (name: string, version: IPnpmVersionSpecifier) => boolean = (
name: string,
versionSpecifier: IPnpmVersionSpecifier
): boolean => {
const version: string = normalizePnpmVersionSpecifier(versionSpecifier);
return !version.includes('link:');
const processCollection = (
collection: Record<string, IPnpmVersionSpecifier>,
optional: boolean
): void => {
const externalDeps: Record<string, IPnpmVersionSpecifier> = {};
for (const [name, versionSpecifier] of Object.entries(collection)) {
const version: string = normalizePnpmVersionSpecifier(versionSpecifier);
if (version.startsWith('link:')) {
// This is a workspace-local dependency; resolve it to an importer key and recurse
const linkPath: string = version.slice('link:'.length);
const importerDir: string = path.dirname(importerKey);
const targetKey: string = Path.convertToSlashes(
path.normalize(path.join(importerDir, linkPath))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot The lockfile paths will always be valid POSIX paths, so use the path.posix.dirname and path.posix.join helpers here rather than normalizing and converting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b37088f. Also corrected the join logic: the link: path is relative to the project folder (the importer key itself), not its parent, so we now use path.posix.join(importerKey, linkPath) rather than path.posix.join(path.posix.dirname(importerKey), linkPath). Both Path.convertToSlashes and path.normalize are removed.

const linkedIntegrities: Map<string, string> | undefined =
this.getIntegrityForImporter(targetKey);
if (linkedIntegrities) {
for (const [dep, integrity] of linkedIntegrities) {
if (!integrityMap!.has(dep)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot Alias integrityMap to a new const local variable immediately after the assignment so that the scope analyzer can tell that it is definitely assigned in this function. Also, since the value of integrity should always be the same, is there any performance benefit to calling .has before .set? At best it optimizes for collisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b37088f. Added const resolvedIntegrityMap: Map<string, string> = new Map() and assigned integrityMap = resolvedIntegrityMap immediately, so all subsequent usages inside the closure use the const. Also removed the .has guard—since the value is always the same, the check only optimized for collisions and isn't needed.

integrityMap!.set(dep, integrity);
}
}
}
} else {
externalDeps[name] = versionSpecifier;
}
}
this._addIntegrities(integrityMap!, externalDeps, optional);
};

if (dependencies) {
this._addIntegrities(integrityMap, dependencies, false, externalFilter);
processCollection(dependencies, false);
}

if (devDependencies) {
this._addIntegrities(integrityMap, devDependencies, false, externalFilter);
processCollection(devDependencies, false);
}

if (optionalDependencies) {
this._addIntegrities(integrityMap, optionalDependencies, true, externalFilter);
processCollection(optionalDependencies, true);
}
}
}
Expand Down Expand Up @@ -1269,14 +1291,9 @@ export class PnpmShrinkwrapFile extends BaseShrinkwrapFile {
private _addIntegrities(
integrityMap: Map<string, string>,
collection: Record<string, IPnpmVersionSpecifier>,
optional: boolean,
filter?: (name: string, version: IPnpmVersionSpecifier) => boolean
optional: boolean
): void {
for (const [name, version] of Object.entries(collection)) {
if (filter && !filter(name, version)) {
continue;
}

const packageId: string = this._getPackageId(name, version);
if (integrityMap.has(packageId)) {
// The entry could already have been added as a nested dependency
Expand Down
108 changes: 108 additions & 0 deletions libraries/rush-lib/src/logic/pnpm/test/PnpmShrinkwrapFile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,114 @@ snapshots:
// because the sub-dependency (bar) resolved to different versions
expect(fooIntegrity1).not.toEqual(fooIntegrity2);
});

it('includes workspace-local link: dependencies by recursing into their importer entries', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot Be sure to cover the following scenarios with test cases:

  1. Workspace project 1 -> Workspace project 2 -> External Dep 1 -> External Dep 2
  2. Workspace project 1 -> External Dep 1 -> Workspace project 2 -> External Dep 2

Note that these may not necessarily be the same dependencies between examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added both scenarios in b37088f:

  • Scenario 1 ("workspace project 1 → workspace project 2 → external dep 1 → external dep 2"): project-1 links to project-2, project-2 depends on ext-a, ext-a transitively depends on ext-b — all four entries verified in project-1's integrity map.
  • Scenario 2 ("workspace project 1 → external dep 1 and workspace project 2 → external dep 2"): project-1 has both a direct external dep (ext-x) and a workspace link dep (project-2), project-2 depends on ext-y — all four entries verified.

Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot , for clarity, in Scenario 2, external dep 1 has a link: dependency that points back into the repository at workspace project 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in ee7bdb3. Scenario 2 now models the correct topology: ../../project-1 depends on ext-a@1.0.0, whose snapshot has project-2: link:../../project-2, and ../../project-2 depends on ext-b@2.0.0.

Also added handling in _addIntegrities for link: deps in package snapshots — since pnpm resolves these relative to the workspace root, the path after link: is used directly as the importer key with no further manipulation.

// This test verifies that link: (workspace-local) dependencies are no longer filtered out.
// The shrinkwrap-deps.json for an importer should include hashes from its workspace
// dependencies' importer sections, all the way down the tree.

// Shrinkwrap with a root importer that depends on a workspace-local package (link:)
// and that local package itself depends on an external package
const shrinkwrapContent: string = `
lockfileVersion: '9.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
importers:
.:
dependencies:
my-lib:
specifier: workspace:*
version: link:projects/my-lib
projects/my-lib:
dependencies:
lodash:
specifier: ^4.17.0
version: 4.17.21
packages:
lodash@4.17.21:
resolution:
integrity: sha512-lodash==
snapshots:
lodash@4.17.21: {}
`;

const shrinkwrapFile = PnpmShrinkwrapFile.loadFromString(shrinkwrapContent, {
subspaceHasNoProjects: false
});

PnpmShrinkwrapFile.clearCache();

const rootIntegrityMap = shrinkwrapFile.getIntegrityForImporter('.');

expect(rootIntegrityMap).toBeDefined();

// The root importer's integrity map should include the linked workspace package's importer entry
expect(rootIntegrityMap!.has('projects/my-lib')).toBe(true);

// It should also include the transitive external dependency of the linked package
expect(rootIntegrityMap!.has('lodash@4.17.21')).toBe(true);

// The integrity map for the workspace package importer itself should also be available
const libIntegrityMap = shrinkwrapFile.getIntegrityForImporter('projects/my-lib');
expect(libIntegrityMap).toBeDefined();
expect(libIntegrityMap!.has('projects/my-lib')).toBe(true);
expect(libIntegrityMap!.has('lodash@4.17.21')).toBe(true);
});

it('produces different hashes when a workspace-local dependency changes', () => {
// This test verifies that changing the dependencies of a workspace-local package
// causes the root importer's integrity to differ.

const baseContent = (lodashVersion: string): string => `
lockfileVersion: '9.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
importers:
.:
dependencies:
my-lib:
specifier: workspace:*
version: link:projects/my-lib
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot With one exception, all valid link: targets will start with at least one ../ in a Rush repository. The exception is if there is a Rush project nested inside of another Rush project, in which case the relative path from one to the other will not start with ../.

The . importer will always be {} unless there are explicitly requested hoisted dependencies.
All valid importer keys will start with ../../ (if subspaces disabled) or ../../../ (if subspaces enabled). This is to enforce that there are no globally hoisted packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in b37088f. All new tests now use realistic Rush importer paths (../../project-1, ../../project-2) and link:../project-2 style relative paths. The . importer is represented as {} as it would be in a real Rush repo.

projects/my-lib:
dependencies:
lodash:
specifier: ^4.17.0
version: ${lodashVersion}
packages:
${lodashVersion === '4.17.21' ? 'lodash@4.17.21' : 'lodash@4.17.20'}:
resolution:
integrity: sha512-lodash${lodashVersion === '4.17.21' ? '21' : '20'}==
snapshots:
${lodashVersion === '4.17.21' ? 'lodash@4.17.21' : 'lodash@4.17.20'}: {}
`;

const shrinkwrapFile1 = PnpmShrinkwrapFile.loadFromString(baseContent('4.17.21'), {
subspaceHasNoProjects: false
});
const shrinkwrapFile2 = PnpmShrinkwrapFile.loadFromString(baseContent('4.17.20'), {
subspaceHasNoProjects: false
});

PnpmShrinkwrapFile.clearCache();

const rootIntegrityMap1 = shrinkwrapFile1.getIntegrityForImporter('.');
const rootIntegrityMap2 = shrinkwrapFile2.getIntegrityForImporter('.');

expect(rootIntegrityMap1).toBeDefined();
expect(rootIntegrityMap2).toBeDefined();

// The self-hash of '.' does NOT change because the root importer object itself is identical
// in both cases (it still references link:projects/my-lib). However, the workspace lib's
// integrity hash should differ because its lodash dependency resolved to a different version.
const libIntegrity1 = rootIntegrityMap1!.get('projects/my-lib');
const libIntegrity2 = rootIntegrityMap2!.get('projects/my-lib');

expect(libIntegrity1).toBeDefined();
expect(libIntegrity2).toBeDefined();
expect(libIntegrity1).not.toEqual(libIntegrity2);
});
});

describe('Check is workspace project modified', () => {
Expand Down
Loading