From 3f22611aac32975b98e7f3ca7b798431f8a494b9 Mon Sep 17 00:00:00 2001 From: michael faith Date: Tue, 12 Sep 2023 11:19:48 -0500 Subject: [PATCH 1/2] fix(@angular-devkit/schematics): running external schematics with yarn pnp This change addresses an issue encountered when running external schematics from a yarn pnp workspace. The function used to resolve a collection json using node used recursion in a way that it effectively walked itself into an exception. Then if the exception is the type it expected, it would keep going. This was flawed in that yarn with pnp throws a different type of error when it failed to load the mis-constructed collection path (e.g. `/node_modules/@schematics/angular/collection.json/package.json`). `ENOTDIR` instead of `MODULE_NOT_FOUND`. This process of intentionally / knowingly walking into an exception seems problematic in general. So, I addressed it by removing the recursion that was used mainly because there's a similar need to construct the collection path from a relative path in the package.json as there is to construct the collection path from a relative path that's passed in. Rather than leaning on the recursion to do this, I added the logic at the time we pull the schematics path from the package, and move on. Since the recursion was removed, the infinite recursion safety check at the start wasn't needed anymore. I've tested this in both yarn pnp and non-pnp environments. --- .../tools/node-module-engine-host.ts | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/angular_devkit/schematics/tools/node-module-engine-host.ts b/packages/angular_devkit/schematics/tools/node-module-engine-host.ts index f03ee5137b6c..aa2e4d06e212 100644 --- a/packages/angular_devkit/schematics/tools/node-module-engine-host.ts +++ b/packages/angular_devkit/schematics/tools/node-module-engine-host.ts @@ -33,18 +33,7 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { super(); } - private resolve(name: string, requester?: string, references = new Set()): string { - if (requester) { - if (references.has(requester)) { - references.add(requester); - throw new Error( - 'Circular schematic reference detected: ' + JSON.stringify(Array.from(references)), - ); - } else { - references.add(requester); - } - } - + private resolve(name: string, requester?: string): string { const relativeBase = requester ? dirname(requester) : process.cwd(); let collectionPath: string | undefined = undefined; @@ -57,6 +46,7 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { }; // Try to resolve as a package + let possibleCollectionPath = name; try { const packageJsonPath = require.resolve(join(name, 'package.json'), resolveOptions); const { schematics } = require(packageJsonPath); @@ -65,7 +55,16 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { throw new NodePackageDoesNotSupportSchematics(name); } - collectionPath = this.resolve(schematics, packageJsonPath, references); + // If this is a relative path to the collection, then create the collection + // path in relation to the package path + if (schematics.startsWith('.')) { + const packageDirectory = dirname(packageJsonPath); + collectionPath = resolve(packageDirectory, schematics); + } + // Otherwise use the path as-is to attempt resolution + else { + possibleCollectionPath = schematics; + } } catch (e) { if ((e as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') { throw e; @@ -75,7 +74,7 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { // If not a package, try to resolve as a file if (!collectionPath) { try { - collectionPath = require.resolve(name, resolveOptions); + collectionPath = require.resolve(possibleCollectionPath, resolveOptions); } catch (e) { if ((e as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') { throw e; From 898ca8492e424368385d7bdb63ce32ef5ae73d23 Mon Sep 17 00:00:00 2001 From: michael faith Date: Thu, 26 Oct 2023 14:52:54 -0500 Subject: [PATCH 2/2] fix(@angular-devkit/schematics): running external schematics with yarn pnp This change addresses an issue encountered when running external schematics from a yarn pnp workspace. The function used to resolve a collection json using node used recursion in a way that it effectively walked itself into an exception. Then, if the exception is the type it expected, it would keep going. This was flawed in that yarn with pnp throws a different type of error when it failed to load the mis-constructed collection path (e.g. `/node_modules/@schematics/angular/collection.json/package.json`). `ENOTDIR` instead of `MODULE_NOT_FOUND`. This process of intentionally / knowingly walking into an exception seems problematic in general. So, I addressed it by first checking if the `schematics` entry in the package is a relative path. If it is, then don't construct the collection path from that. If entry is not relative, then assume it's pointing at another package and we need to recurse to get to the actual collection path. I've tested this in both yarn pnp and non-pnp environments. --- .../tools/node-module-engine-host.ts | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/angular_devkit/schematics/tools/node-module-engine-host.ts b/packages/angular_devkit/schematics/tools/node-module-engine-host.ts index aa2e4d06e212..246df24d2dd2 100644 --- a/packages/angular_devkit/schematics/tools/node-module-engine-host.ts +++ b/packages/angular_devkit/schematics/tools/node-module-engine-host.ts @@ -33,7 +33,19 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { super(); } - private resolve(name: string, requester?: string): string { + private resolve(name: string, requester?: string, references = new Set()): string { + // Keep track of the package requesting the schematic, in order to avoid infinite recursion + if (requester) { + if (references.has(requester)) { + references.add(requester); + throw new Error( + 'Circular schematic reference detected: ' + JSON.stringify(Array.from(references)), + ); + } else { + references.add(requester); + } + } + const relativeBase = requester ? dirname(requester) : process.cwd(); let collectionPath: string | undefined = undefined; @@ -46,7 +58,6 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { }; // Try to resolve as a package - let possibleCollectionPath = name; try { const packageJsonPath = require.resolve(join(name, 'package.json'), resolveOptions); const { schematics } = require(packageJsonPath); @@ -61,9 +72,9 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { const packageDirectory = dirname(packageJsonPath); collectionPath = resolve(packageDirectory, schematics); } - // Otherwise use the path as-is to attempt resolution + // Otherwise treat this as a package, and recurse to find the collection path else { - possibleCollectionPath = schematics; + collectionPath = this.resolve(schematics, packageJsonPath, references); } } catch (e) { if ((e as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') { @@ -74,7 +85,7 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { // If not a package, try to resolve as a file if (!collectionPath) { try { - collectionPath = require.resolve(possibleCollectionPath, resolveOptions); + collectionPath = require.resolve(name, resolveOptions); } catch (e) { if ((e as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') { throw e;