BUG: Fix inference of node_modules for loader prefixes. (#99)

Part of #79 - BUG: A loader in the `identifier` field would incorrectly have all modules inferred "as a `node_modules` file", even if not. Implements a naive loader stripping heuristic to correctly assess if `node_modules` or real application source. - Optimizes internal calls to `_isNodeModules()` from 2 to 1 for better performance.
FormidableLabs · Dec 12, 2018 · 59294a3 · 59294a3
1 parent 2ea66cd
commit 59294a3
Show file tree

Hide file tree

Showing 4 changed files with 129 additions and 76 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -1,6 +1,11 @@
 History
 =======
 
+## UNRELEASED
+
+- BUG: A loader in the `identifier` field would incorrectly have all modules inferred "as a `node_modules` file", even if not. Implements a naive loader stripping heuristic to correctly assess if `node_modules` or real application source.
+- Optimizes internal calls to `_isNodeModules()` from 2 to 1 for better performance.
+
 ## 4.1.0
 
 - Add `emitHandler` option to `DuplicatesPlugin` to allow customized output.

diff --git a/README.md b/README.md
@@ -16,6 +16,7 @@ It is also the engine for the handy [`webpack-dashboard`](https://github.com/For
   - [`duplicates`](#duplicates)
   - [`versions`](#versions)
   - [`sizes`](#sizes)
+- [Notes, tips, tricks](#notes-tips-tricks)
 - [Other useful tools](#other-useful-tools)
 
 ## Plugin
@@ -467,6 +468,14 @@ inspectpack --action=sizes
 
 The sizes reported are most likely of the uncompressed source of each module. Because `inspectpack` relies on the `stats` object output, the information reported in the sizes action reflects at what point the `stats` object was generated. For example, using the recommended `webpack-stats-plugin`, the source information would be after all loader processing, but potentially before any webpack plugins. Thus, the resultant, _actual_ size of a given module in your ultimate bundle could be bigger (e.g., in a development bundle with webpack-inserted comments and imports) or smaller (e.g., your bundle is minified and gzipped).
 
+## Notes, tips, tricks
+
+### Special characters in file paths
+
+Webpack loaders use a special syntax for loaders with `?` and `!` characters that will end up in the stats object `identifier` field (e.g., `/PATH/TO/node_modules/css-loader/index.js??ref--7-1!/PATH/TO/node_modules/postcss-loader/lib/index.js??ref--7-2!/PATH/TO/src/bar/my-style.css"`) for a given module item.
+
+We currently use a very naive solution to determine the "true" asset name by just stripping off everything before the last `?`/`!` character. There are technically some potential use cases (e.g. those characters in real file paths) that might not be correctly handled. We have a [tracking ticket](https://github.com/FormidableLabs/inspectpack/issues/98) for folks to comment on if you're hitting any issues.
+
 ## Other useful tools
 
 Other tools that inspect Webpack bundles:

diff --git a/src/lib/actions/base.ts b/src/lib/actions/base.ts
@@ -45,18 +45,30 @@ export const nodeModulesParts = (name: string) => toPosixPath(name).split(NM_RE)
 // True if name is part of a `node_modules` path.
 export const _isNodeModules = (name: string): boolean => nodeModulesParts(name).length > 1;
 
+// Naively remove any prefix portion of an identifier, namely:
+// - `REMOVE?KEEP`
+// - `REMOVE!KEEP`
+export const _normalizeIdentifier = (name: string): string => {
+  const lastBang = name.lastIndexOf("!");
+  const lastQuestion = name.lastIndexOf("?");
+  const prefixEnd = Math.max(lastBang, lastQuestion);
+
+  // Remove prefix here.
+  if (prefixEnd > -1) {
+    return name.substr(prefixEnd + 1);
+  }
+
+  return name;
+};
+
 // Convert a `node_modules` name to a base name.
 //
+// **Note**: Assumes only passed `node_modules` values.
+//
 // Normalizations:
 // - Remove starting path if `./`
 // - Switch Windows paths to Mac/Unix style.
-// - Non-`node_modules` sources (e.g. "your" sources) return `null`.
 export const _getBaseName = (name: string): string | null => {
-  // Not in `node_modules`.
-  if (!_isNodeModules(name)) {
-    return null;
-  }
-
   // Slice to just after last occurrence of node_modules.
   const parts = nodeModulesParts(name);
   const lastName = parts[parts.length - 1];
@@ -133,42 +145,51 @@ export abstract class Action {
           // Add in any parent chunks and ensure unique array.
           const chunks = Array.from(new Set(mod.chunks.concat(parentChunks || [])));
 
-          if (RWebpackStatsModuleSource.decode(mod).isRight()) {
-            // Easy case -- a normal source code module.
-            const srcMod = mod as IWebpackStatsModuleSource;
-            const { identifier, size, source } = srcMod;
-
-            return list.concat([{
-              baseName: _getBaseName(identifier),
-              chunks,
-              identifier,
-              isNodeModules: _isNodeModules(identifier),
-              isSynthetic: false,
-              size,
-              source,
-            }]);
-          } else if (RWebpackStatsModuleModules.decode(mod).isRight()) {
+          // Fields
+          let isSynthetic = false;
+          let source = null;
+          let identifier;
+          let size;
+
+          if (RWebpackStatsModuleModules.decode(mod).isRight()) {
             // Recursive case -- more modules.
             const modsMod = mod as IWebpackStatsModuleModules;
 
+            // Return and recurse.
             return list.concat(this.getSourceMods(modsMod.modules, chunks));
+
+          } else if (RWebpackStatsModuleSource.decode(mod).isRight()) {
+            // Easy case -- a normal source code module.
+            const srcMod = mod as IWebpackStatsModuleSource;
+            identifier = srcMod.identifier;
+            size = srcMod.size;
+            source = srcMod.source;
+
           } else if (RWebpackStatsModuleSynthetic.decode(mod).isRight()) {
             // Catch-all case -- a module without modules or source.
             const syntheticMod = mod as IWebpackStatsModuleSynthetic;
-            const { identifier, size } = syntheticMod;
-
-            return list.concat([{
-              baseName: _getBaseName(identifier),
-              chunks,
-              identifier,
-              isNodeModules: _isNodeModules(identifier),
-              isSynthetic: true,
-              size,
-              source: null,
-            }]);
+            identifier = syntheticMod.identifier;
+            size = syntheticMod.size;
+            isSynthetic = true;
+
+          } else {
+            throw new Error(`Cannot match to known module type: ${JSON.stringify(mod)}`);
           }
 
-          throw new Error(`Cannot match to known module type: ${JSON.stringify(mod)}`);
+          // We've now got a single entry to prepare and add.
+          const normalizedId = _normalizeIdentifier(identifier as string);
+          const isNodeModules = _isNodeModules(normalizedId);
+          const baseName = isNodeModules ? _getBaseName(normalizedId) : null;
+
+          return list.concat([{
+            baseName,
+            chunks,
+            identifier,
+            isNodeModules,
+            isSynthetic,
+            size,
+            source,
+          }]);
         },
         [],
       )

diff --git a/test/lib/actions/base.spec.ts b/test/lib/actions/base.spec.ts
@@ -1,48 +1,6 @@
-import { _getBaseName, _isNodeModules } from "../../../src/lib/actions/base";
+import { _getBaseName, _isNodeModules, _normalizeIdentifier } from "../../../src/lib/actions/base";
 
 describe("lib/actions/base", () => {
-  describe("_getBaseName", () => {
-    it("handles non-node_modules files", () => {
-      expect(_getBaseName("")).to.equal(null);
-      expect(_getBaseName("foo.js")).to.equal(null);
-      expect(_getBaseName("./foo.js")).to.equal(null);
-      expect(_getBaseName(".\\foo.js")).to.equal(null);
-      expect(_getBaseName("bar/foo.js")).to.equal(null);
-      expect(_getBaseName("bar\\foo.js")).to.equal(null);
-      expect(_getBaseName("node_modulesM/bar/foo.js")).to.equal(null);
-      expect(_getBaseName("node_modulesM\\bar\\foo.js")).to.equal(null);
-    });
-
-    it("removes node_modules", () => {
-      expect(_getBaseName("./node_modules/foo.js")).to.equal("foo.js");
-      expect(_getBaseName(".\\node_modules\\foo.js")).to.equal("foo.js");
-      expect(_getBaseName("node_modules/bar/foo.js")).to.equal("bar/foo.js");
-      expect(_getBaseName("node_modules\\bar\\foo.js")).to.equal("bar/foo.js");
-    });
-
-    it("removes repeated node_modules", () => {
-      expect(_getBaseName("./node_modules/baz/node_modules/foo.js")).to.equal("foo.js");
-      expect(_getBaseName(".\\node_modules\\baz\\node_modules\\foo.js")).to.equal("foo.js");
-      expect(_getBaseName("bruh/node_modules/bar/foo.js")).to.equal("bar/foo.js");
-      expect(_getBaseName("bruh\\node_modules\\bar\\foo.js")).to.equal("bar/foo.js");
-    });
-
-    it("handles synthetic modules", () => {
-      expect(_getBaseName("node_modules/moment/locale sync /es/"))
-        .to.equal("moment/locale sync /es/");
-      expect(_getBaseName("node_modules\\moment/locale sync /es/"))
-        .to.equal("moment/locale sync /es/");
-    });
-
-    // All of this behavior is negotiable.
-    it("handles weird cases that should never come up", () => {
-      expect(_getBaseName("node_modules")).to.equal("");
-      expect(_getBaseName("node_modules/")).to.equal("");
-      expect(_getBaseName("./node_modules")).to.equal("");
-      expect(_getBaseName("./foo/bar/node_modules")).to.equal("");
-    });
-  });
-
   describe("_isNodeModules", () => {
     it("handles base cases", () => {
       expect(_isNodeModules("")).to.equal(false);
@@ -77,4 +35,64 @@ describe("lib/actions/base", () => {
       expect(_isNodeModules("./foo/bar/node_modules")).to.equal(true);
     });
   });
+
+  describe("_normalizeIdentifier", () => {
+    it("handles base cases", () => {
+      expect(_normalizeIdentifier("")).to.equal("");
+      expect(_normalizeIdentifier("foo.js")).to.equal("foo.js");
+      expect(_normalizeIdentifier("/foo.js")).to.equal("/foo.js");
+      expect(_normalizeIdentifier("\\foo.js")).to.equal("\\foo.js");
+      expect(_normalizeIdentifier("bar/foo.js")).to.equal("bar/foo.js");
+      expect(_normalizeIdentifier("bar\\foo.js")).to.equal("bar\\foo.js");
+      expect(_normalizeIdentifier("/bar/foo.js")).to.equal("/bar/foo.js");
+      expect(_normalizeIdentifier("x:\\bar\\foo.js")).to.equal("x:\\bar\\foo.js");
+    });
+
+    // tslint:disable max-line-length
+    it("handles loaders", () => {
+      expect(_normalizeIdentifier("/PATH/TO/node_modules/css-loader/lib/css-base.js"))
+        .to.equal("/PATH/TO/node_modules/css-loader/lib/css-base.js");
+      expect(_normalizeIdentifier("/PATH/TO/node_modules/next/dist/build/webpack/loaders/next-babel-loader.js??ref--4!/PATH/TO/node_modules/pkg/foo.js"))
+        .to.equal("/PATH/TO/node_modules/pkg/foo.js");
+      expect(_normalizeIdentifier("/PATH/TO/node_modules/css-loader/index.js??ref--7-1!/PATH/TO/node_modules/postcss-loader/lib/index.js??ref--7-2!/PATH/TO/node_modules/@scope/foo/package.css"))
+        .to.equal("/PATH/TO/node_modules/@scope/foo/package.css");
+
+      expect(_normalizeIdentifier("/PATH/TO/node_modules/next/dist/build/webpack/loaders/next-babel-loader.js??ref--4!/PATH/TO/src/modules/debug/foo.js"))
+        .to.equal("/PATH/TO/src/modules/debug/foo.js");
+      expect(_normalizeIdentifier("/PATH/TO/node_modules/css-loader/index.js??ref--7-1!/PATH/TO/node_modules/postcss-loader/lib/index.js??ref--7-2!/PATH/TO/src/bar/my-style.css"))
+        .to.equal("/PATH/TO/src/bar/my-style.css");
+    });
+    // tslint:enable max-line-length
+  });
+
+  describe("_getBaseName", () => {
+    it("removes node_modules", () => {
+      expect(_getBaseName("./node_modules/foo.js")).to.equal("foo.js");
+      expect(_getBaseName(".\\node_modules\\foo.js")).to.equal("foo.js");
+      expect(_getBaseName("node_modules/bar/foo.js")).to.equal("bar/foo.js");
+      expect(_getBaseName("node_modules\\bar\\foo.js")).to.equal("bar/foo.js");
+    });
+
+    it("removes repeated node_modules", () => {
+      expect(_getBaseName("./node_modules/baz/node_modules/foo.js")).to.equal("foo.js");
+      expect(_getBaseName(".\\node_modules\\baz\\node_modules\\foo.js")).to.equal("foo.js");
+      expect(_getBaseName("bruh/node_modules/bar/foo.js")).to.equal("bar/foo.js");
+      expect(_getBaseName("bruh\\node_modules\\bar\\foo.js")).to.equal("bar/foo.js");
+    });
+
+    it("handles synthetic modules", () => {
+      expect(_getBaseName("node_modules/moment/locale sync /es/"))
+        .to.equal("moment/locale sync /es/");
+      expect(_getBaseName("node_modules\\moment/locale sync /es/"))
+        .to.equal("moment/locale sync /es/");
+    });
+
+    // All of this behavior is negotiable.
+    it("handles weird cases that should never come up", () => {
+      expect(_getBaseName("node_modules")).to.equal("");
+      expect(_getBaseName("node_modules/")).to.equal("");
+      expect(_getBaseName("./node_modules")).to.equal("");
+      expect(_getBaseName("./foo/bar/node_modules")).to.equal("");
+    });
+  });
 });