From 5d19af9051e3b513827665c9a8d3151063efe929 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 21 Dec 2021 11:06:25 +0100 Subject: [PATCH 1/5] fix(pacmak): fails on bundled dependency without entry point `projen` uses a bundled dependency called `shx`, and this library does not have an entry point. It exposes some command-line tools and nothing else. This conflicts with our dependency finding mechanism, which performs a `require()` on the library to find it. Ignore bundled dependencies in `pacmak`, it never needs to pack them individually anyway. --- packages/jsii-pacmak/lib/dependency-graph.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/jsii-pacmak/lib/dependency-graph.ts b/packages/jsii-pacmak/lib/dependency-graph.ts index d00bdae639..ba67d8f562 100644 --- a/packages/jsii-pacmak/lib/dependency-graph.ts +++ b/packages/jsii-pacmak/lib/dependency-graph.ts @@ -58,6 +58,7 @@ export interface TraverseDependencyGraphHost { export interface PackageJson { readonly dependencies?: { readonly [name: string]: string }; readonly peerDependencies?: { readonly [name: string]: string }; + readonly bundledDependencies?: string[]; readonly [key: string]: unknown; } @@ -88,7 +89,12 @@ async function real$traverseDependencyGraph( ]); return Promise.all( Array.from(deps) - .filter((m) => !util.isBuiltinModule(m)) + // No need to pacmak the dependency if it's built-in, or if it's bundled + .filter( + (m) => + !util.isBuiltinModule(m) && + !(meta.bundledDependencies ?? []).includes(m), + ) .map(async (dep) => { const dependencyDir = await host.findDependencyDirectory( dep, From 80da5d3ddbf549bb48a1356dc8898757149ac2ee Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 21 Dec 2021 11:15:30 +0100 Subject: [PATCH 2/5] Add test --- .../jsii-pacmak/test/dependency-graph.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/jsii-pacmak/test/dependency-graph.test.ts b/packages/jsii-pacmak/test/dependency-graph.test.ts index e46446c167..ea153034ea 100644 --- a/packages/jsii-pacmak/test/dependency-graph.test.ts +++ b/packages/jsii-pacmak/test/dependency-graph.test.ts @@ -113,3 +113,41 @@ test('stops traversing when callback returns false', async () => { expect(mockHost.readJson).toHaveBeenCalledTimes(2); expect(mockHost.findDependencyDirectory).toHaveBeenCalledTimes(1); }); + +test('dont call findDependencyDirectory for bundledDependencies', async () => { + // GIVEN the following package dependency graph: + // A -> B -> C + const packages: Record = { + A: { + root: join(tmpdir(), 'A'), + meta: { dependencies: { B: '*' }, bundledDependencies: ['B'] }, + }, + }; + + const cb: Callback = jest.fn().mockName('callback').mockReturnValue(true); + + mockHost.readJson.mockImplementation((file) => { + const result = Object.values(packages).find( + ({ root }) => file === join(root, 'package.json'), + )?.meta; + return result != null + ? Promise.resolve(result) + : Promise.reject(new Error(`Unexpected file access: ${file}`)); + }); + + mockHost.findDependencyDirectory.mockImplementation(async (dep, _dir) => { + const result = packages[dep]?.root; + if (result == null) { + throw new Error(`Unknown dependency: ${dep}`); + } + return result; + }); + + // WHEN + await expect( + traverseDependencyGraph(packages.A.root, cb, mockHost), + ).resolves.not.toThrow(); + + // THEN + expect(mockHost.findDependencyDirectory).not.toHaveBeenCalled(); +}); From e56e7bb48eeba42aa3301c9d51d3ec223004339f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 21 Dec 2021 15:57:02 +0100 Subject: [PATCH 3/5] Add bundleDependencies --- packages/jsii-pacmak/lib/dependency-graph.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/jsii-pacmak/lib/dependency-graph.ts b/packages/jsii-pacmak/lib/dependency-graph.ts index ba67d8f562..c24b7d0ff9 100644 --- a/packages/jsii-pacmak/lib/dependency-graph.ts +++ b/packages/jsii-pacmak/lib/dependency-graph.ts @@ -58,6 +58,7 @@ export interface TraverseDependencyGraphHost { export interface PackageJson { readonly dependencies?: { readonly [name: string]: string }; readonly peerDependencies?: { readonly [name: string]: string }; + readonly bundleDependencies?: string[]; readonly bundledDependencies?: string[]; readonly [key: string]: unknown; @@ -93,7 +94,7 @@ async function real$traverseDependencyGraph( .filter( (m) => !util.isBuiltinModule(m) && - !(meta.bundledDependencies ?? []).includes(m), + (meta.bundledDependencies ?? meta.bundleDependencies)?.includes(m), ) .map(async (dep) => { const dependencyDir = await host.findDependencyDirectory( From 4a8dbf4cb3cde3629d86eb2997c1c1856de1ca31 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 22 Dec 2021 10:04:42 +0100 Subject: [PATCH 4/5] Invert condition because I am silly --- packages/jsii-pacmak/lib/dependency-graph.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jsii-pacmak/lib/dependency-graph.ts b/packages/jsii-pacmak/lib/dependency-graph.ts index c24b7d0ff9..1296c50d43 100644 --- a/packages/jsii-pacmak/lib/dependency-graph.ts +++ b/packages/jsii-pacmak/lib/dependency-graph.ts @@ -94,7 +94,7 @@ async function real$traverseDependencyGraph( .filter( (m) => !util.isBuiltinModule(m) && - (meta.bundledDependencies ?? meta.bundleDependencies)?.includes(m), + !(meta.bundledDependencies ?? meta.bundleDependencies)?.includes(m), ) .map(async (dep) => { const dependencyDir = await host.findDependencyDirectory( From ab6b740fe81b4b479bdcb22ebbab537a65bc81fc Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 22 Dec 2021 11:12:50 +0100 Subject: [PATCH 5/5] Respect bundleDependencies and bundledDependencies both --- packages/jsii-pacmak/lib/dependency-graph.ts | 3 +- .../jsii-pacmak/test/dependency-graph.test.ts | 76 +++++++++++++++---- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/packages/jsii-pacmak/lib/dependency-graph.ts b/packages/jsii-pacmak/lib/dependency-graph.ts index 1296c50d43..2661bdb8d9 100644 --- a/packages/jsii-pacmak/lib/dependency-graph.ts +++ b/packages/jsii-pacmak/lib/dependency-graph.ts @@ -94,7 +94,8 @@ async function real$traverseDependencyGraph( .filter( (m) => !util.isBuiltinModule(m) && - !(meta.bundledDependencies ?? meta.bundleDependencies)?.includes(m), + !meta.bundledDependencies?.includes(m) && + !meta.bundleDependencies?.includes(m), ) .map(async (dep) => { const dependencyDir = await host.findDependencyDirectory( diff --git a/packages/jsii-pacmak/test/dependency-graph.test.ts b/packages/jsii-pacmak/test/dependency-graph.test.ts index ea153034ea..cf0eac3a97 100644 --- a/packages/jsii-pacmak/test/dependency-graph.test.ts +++ b/packages/jsii-pacmak/test/dependency-graph.test.ts @@ -115,8 +115,6 @@ test('stops traversing when callback returns false', async () => { }); test('dont call findDependencyDirectory for bundledDependencies', async () => { - // GIVEN the following package dependency graph: - // A -> B -> C const packages: Record = { A: { root: join(tmpdir(), 'A'), @@ -126,8 +124,68 @@ test('dont call findDependencyDirectory for bundledDependencies', async () => { const cb: Callback = jest.fn().mockName('callback').mockReturnValue(true); + fakeReadJson(packages); + + // WHEN + await expect( + traverseDependencyGraph(packages.A.root, cb, mockHost), + ).resolves.not.toThrow(); + + // THEN + expect(mockHost.findDependencyDirectory).not.toHaveBeenCalled(); +}); + +test('dont call findDependencyDirectory for bundleDependencies', async () => { + const packages: Record = { + A: { + root: join(tmpdir(), 'A'), + meta: { dependencies: { B: '*' }, bundleDependencies: ['B'] }, + }, + }; + + const cb: Callback = jest.fn().mockName('callback').mockReturnValue(true); + + fakeReadJson(packages); + + // WHEN + await expect( + traverseDependencyGraph(packages.A.root, cb, mockHost), + ).resolves.not.toThrow(); + + // THEN + expect(mockHost.findDependencyDirectory).not.toHaveBeenCalled(); +}); + +test('dont call findDependencyDirectory for bundleDependencies AND bundledDependencies', async () => { + const packages: Record = { + A: { + root: join(tmpdir(), 'A'), + meta: { + dependencies: { B: '*', C: '*' }, + bundleDependencies: ['B'], + bundledDependencies: ['C'], + }, + }, + }; + + const cb: Callback = jest.fn().mockName('callback').mockReturnValue(true); + + fakeReadJson(packages); + + // WHEN + await expect( + traverseDependencyGraph(packages.A.root, cb, mockHost), + ).resolves.not.toThrow(); + + // THEN + expect(mockHost.findDependencyDirectory).not.toHaveBeenCalled(); +}); + +function fakeReadJson( + fakePackages: Record, +) { mockHost.readJson.mockImplementation((file) => { - const result = Object.values(packages).find( + const result = Object.values(fakePackages).find( ({ root }) => file === join(root, 'package.json'), )?.meta; return result != null @@ -136,18 +194,10 @@ test('dont call findDependencyDirectory for bundledDependencies', async () => { }); mockHost.findDependencyDirectory.mockImplementation(async (dep, _dir) => { - const result = packages[dep]?.root; + const result = fakePackages[dep]?.root; if (result == null) { throw new Error(`Unknown dependency: ${dep}`); } return result; }); - - // WHEN - await expect( - traverseDependencyGraph(packages.A.root, cb, mockHost), - ).resolves.not.toThrow(); - - // THEN - expect(mockHost.findDependencyDirectory).not.toHaveBeenCalled(); -}); +}