Repository: sverweij/dependency-cruiser
Pull request #865 (2023-11-12 12:53:48)

refactor(resolve): hands over preserveSymlink processing to EHR
## Description

- removes most preserveSymlink processing in favor of passing the preserveSymlink option  to enhanced-resolve (which does this out of the box).
- unwraps the swallow-tail in the resolve index & refactors the yarn/ berry `resolveVirtual` things
- It's now properly reflected EHR's `symlinks` === `!preserveSymlink`

## Motivation and Context

- Simplifies our code
- By moving it to enhanced-resolve the properly resolved module name is available earlier in the process. This will make our live easier when we'd ever want to use that e.g. for workspaces (which are nothing but symlinks from node_modules to local folders).

## How Has This Been Tested?

- [x] green ci
- [x] adapted automatic non-regression test

## Types of changes

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation only change
- [x] Refactor (non-breaking change which fixes an issue without changing functionality)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist

- [x] :book:

  - My change doesn't require a documentation update, or ...
  - it _does_ and I have updated it

- [x] :balance_scale:
  - The contribution will be subject to [The MIT license](https://github.com/sverweij/dependency-cruiser/blob/main/LICENSE), and I'm OK with that.
  - The contribution is my own original work.
  - I am ok with the stuff in [**CONTRIBUTING.md**](https://github.com/sverweij/dependency-cruiser/blob/main/.github/CONTRIBUTING.md).


codeclimate[bot]: Code Climate has analyzed commit 8d48386f and detected **0 issues** on this pull request.

View more on [Code Climate](https://codeclimate.com/github/sverweij/dependency-cruiser/pull/865).

codeclimate[bot]: Code Climate has analyzed commit 0de3b707 and detected **0 issues** on this pull request.

View more on [Code Climate](https://codeclimate.com/github/sverweij/dependency-cruiser/pull/865).


Patch:
diff --git a/src/extract/resolve/index.mjs b/src/extract/resolve/index.mjs
index d71801ab..49d91467 100644
--- a/src/extract/resolve/index.mjs
+++ b/src/extract/resolve/index.mjs
@@ -1,4 +1,3 @@
-import { realpathSync } from "node:fs";
 import { extname, resolve as path_resolve, relative } from "node:path";
 import monkeyPatchedModule from "node:module";
 import { isRelativeModuleName } from "./module-classifiers.mjs";
@@ -54,7 +53,7 @@ function canBeResolvedToTsVariant(pModuleName) {
 function isTypeScriptIshExtension(pModuleName) {
   return [".ts", ".tsx", ".cts", ".mts"].includes(extname(pModuleName));
 }
-function resolveYarnVirtual(pPath) {
+function resolveYarnVirtual(pBaseDirectory, pPath) {
   const pnpAPI = (monkeyPatchedModule?.findPnpApi ?? (() => false))(pPath);
 
   // the pnp api only works in plug'n play environments, and resolveVirtual
@@ -63,7 +62,15 @@ function resolveYarnVirtual(pPath) {
   // cover it in a separate integration test.
   /* c8 ignore start */
   if (pnpAPI && (pnpAPI?.VERSIONS?.resolveVirtual ?? 0) === 1) {
-    return pnpAPI.resolveVirtual(path_resolve(pPath)) || pPath;
+    // resolveVirtual takes absolute paths, hence the path.resolve:
+    const lResolvedAbsolute = path_resolve(pBaseDirectory, pPath);
+    const lResolvedVirtual = pnpAPI.resolveVirtual(lResolvedAbsolute);
+    if (lResolvedVirtual) {
+      const lResolvedRelative = relative(pBaseDirectory, lResolvedVirtual);
+      // in win32 environments resolveVirtual might return win32 paths,
+      // so we have to convert them to posix paths again
+      return pathToPosix(lResolvedRelative);
+    }
   }
   /* c8 ignore stop */
   return pPath;
@@ -193,33 +200,21 @@ export default function resolve(
     ),
   };
 
-  if (
-    !pResolveOptions.symlinks &&
-    !lResolvedDependency.coreModule &&
-    !lResolvedDependency.couldNotResolve
-  ) {
-    try {
-      lResolvedDependency.resolved = pathToPosix(
-        relative(
-          pBaseDirectory,
-          realpathSync(
-            resolveYarnVirtual(
-              path_resolve(
-                pBaseDirectory,
-                /* enhanced-resolve inserts a NULL character in front of any `#` 
-                   character. This wonky replace undoes that so the filename
-                   again corresponds with a real file on disk
-                 */
-                // eslint-disable-next-line no-control-regex
-                lResolvedDependency.resolved.replace(/\u0000#/g, "#"),
-              ),
-            ),
-          ),
-        ),
-      );
-    } catch (pError) {
-      lResolvedDependency.couldNotResolve = true;
-    }
+  if (!lResolvedDependency.coreModule && !lResolvedDependency.couldNotResolve) {
+    // enhanced-resolve inserts a NULL character in front of any `#` character.
+    // This wonky replace corrects that that so the filename again corresponds
+    // with a real file on disk
+    const lResolvedEHRCorrected = lResolvedDependency.resolved.replace(
+      // eslint-disable-next-line no-control-regex
+      /\u0000#/g,
+      "#",
+    );
+    const lResolvedYarnVirtual = resolveYarnVirtual(
+      pBaseDirectory,
+      lResolvedEHRCorrected,
+    );
+
+    lResolvedDependency.resolved = lResolvedYarnVirtual;
   }
   return lResolvedDependency;
 }
diff --git a/src/extract/resolve/resolve.mjs b/src/extract/resolve/resolve.mjs
index 41889d0a..bde94a42 100644
--- a/src/extract/resolve/resolve.mjs
+++ b/src/extract/resolve/resolve.mjs
@@ -5,20 +5,12 @@ import pathToPosix from "#utl/path-to-posix.mjs";
 let gResolvers = {};
 let gInitialized = {};
 
-function init(pResolveOptions, pCachingContext) {
-  if (!gInitialized[pCachingContext] || pResolveOptions.bustTheCache) {
+function init(pEHResolveOptions, pCachingContext) {
+  if (!gInitialized[pCachingContext] || pEHResolveOptions.bustTheCache) {
     // assuming the cached file system here
-    pResolveOptions.fileSystem.purge();
+    pEHResolveOptions.fileSystem.purge();
     gResolvers[pCachingContext] =
-      enhancedResolve.ResolverFactory.createResolver({
-        ...pResolveOptions,
-        // we're doing that ourselves for now. We can't set this in
-        // 'normalize' because we actively use resolveOptions.symlinks
-        // with our own symlink resolution thing, so we need to override
-        // it here locally so even when it is passed as true we skip
-        // ehr's capabilities in this and still do it ourselves.
-        symlinks: false,
-      });
+      enhancedResolve.ResolverFactory.createResolver(pEHResolveOptions);
     /* eslint security/detect-object-injection:0 */
     gInitialized[pCachingContext] = true;
   }
diff --git a/src/main/resolve-options/normalize.mjs b/src/main/resolve-options/normalize.mjs
index 2f3d9c60..302d54bf 100644
--- a/src/main/resolve-options/normalize.mjs
+++ b/src/main/resolve-options/normalize.mjs
@@ -10,14 +10,7 @@ import {
 const DEFAULT_CACHE_DURATION = 4000;
 /** @type {Partial<import("../../../types/dependency-cruiser").IResolveOptions>} */
 const DEFAULT_RESOLVE_OPTIONS = {
-  // for later: check semantics of enhanced-resolve symlinks and
-  // node's preserveSymlinks. They seem to be
-  // symlink === !preserveSymlinks - but using it that way
-  // breaks backwards compatibility
-  //
-  // Someday we'll rely on this and remove the code that manually
-  // does this in extract/resolve/index.js
-  symlinks: false,
+  symlinks: true,
   // if a webpack config overrides extensions, there's probably
   // good cause. The scannableExtensions are an educated guess
   // anyway, that works well in most circumstances.
@@ -145,13 +138,12 @@ export default async function normalizeResolveOptions(
   // eslint-disable-next-line no-return-await
   return await compileResolveOptions(
     {
-      /*
-        for later: check semantics of enhanced-resolve symlinks and
-        node's preserveSymlinks. They seem to be
-        symlink === !preserveSymlinks - but using it that way
-        breaks backwards compatibility
-      */
-      symlinks: pOptions?.preserveSymlinks ?? null,
+      // EnhancedResolve's symlinks:
+      // - true => symlinks are followed (vv)
+      // node's --preserve-symlinks:
+      // - true => symlinks are NOT followed (vv)
+      // => symlinks = !preserveSymlinks
+      symlinks: !pOptions?.preserveSymlinks,
       tsConfig: pOptions?.ruleSet?.options?.tsConfig?.fileName ?? null,
 
       /* squirrel the externalModuleResolutionStrategy and combinedDependencies


Test patch:
diff --git a/test/main/resolve-options/normalize.spec.mjs b/test/main/resolve-options/normalize.spec.mjs
index 2621eb3b..714f4922 100644
--- a/test/main/resolve-options/normalize.spec.mjs
+++ b/test/main/resolve-options/normalize.spec.mjs
@@ -21,7 +21,7 @@ describe("[I] main/resolve-options/normalize", () => {
     );
 
     equal(Object.keys(lNormalizedOptions).length, lDefaultNoOfResolveOptions);
-    equal(lNormalizedOptions.symlinks, false);
+    equal(lNormalizedOptions.symlinks, true);
     equal(lNormalizedOptions.tsConfig, null);
     equal(lNormalizedOptions.combinedDependencies, false);
     ok(lNormalizedOptions.hasOwnProperty("extensions"));
@@ -39,7 +39,7 @@ describe("[I] main/resolve-options/normalize", () => {
     );
 
     equal(Object.keys(lNormalizedOptions).length, lDefaultNoOfResolveOptions);
-    equal(lNormalizedOptions.symlinks, false);
+    equal(lNormalizedOptions.symlinks, true);
     equal(lNormalizedOptions.tsConfig, null);
     equal(lNormalizedOptions.combinedDependencies, false);
     ok(lNormalizedOptions.hasOwnProperty("extensions"));
@@ -61,7 +61,7 @@ describe("[I] main/resolve-options/normalize", () => {
       Object.keys(lNormalizedOptions).length,
       lDefaultNoOfResolveOptions + 1,
     );
-    equal(lNormalizedOptions.symlinks, false);
+    equal(lNormalizedOptions.symlinks, true);
     equal(lNormalizedOptions.tsConfig, TEST_TSCONFIG);
     equal(lNormalizedOptions.combinedDependencies, false);
     ok(lNormalizedOptions.hasOwnProperty("extensions"));
@@ -83,7 +83,7 @@ describe("[I] main/resolve-options/normalize", () => {
       Object.keys(lNormalizedOptions).length,
       lDefaultNoOfResolveOptions + 1,
     );
-    equal(lNormalizedOptions.symlinks, false);
+    equal(lNormalizedOptions.symlinks, true);
     equal(lNormalizedOptions.tsConfig, TEST_TSCONFIG);
     equal(lNormalizedOptions.combinedDependencies, false);
     ok(lNormalizedOptions.hasOwnProperty("extensions"));