fix: Report.isOneLineRequire should be true if single line LogicalExpression assignment (#205)

---------

Co-authored-by: Thomas.G <gentilhomme.thomas@gmail.com>

Author: jean-michelet

index.js

@@ -10,6 +10,7 @@ import isMinified from "is-minified-code";
 import { SourceFile } from "./src/SourceFile.js";
 import { SourceParser } from "./src/SourceParser.js";
 import { warnings } from "./src/warnings.js";
+import { isOneLineExpressionExport } from "./src/utils.js";
 
 export function runASTAnalysis(
   str,
@@ -46,7 +47,7 @@ export function runASTAnalysis(
   return {
     ...source.getResult(isMinified),
     dependencies: source.dependencies,
-    isOneLineRequire: body.length <= 1 && source.dependencies.size <= 1
+    isOneLineRequire: isOneLineExpressionExport(body)
   };
 }
 

src/utils.js

@@ -33,6 +33,58 @@ export function isUnsafeCallee(node) {
   return [false, null];
 }
 
+export function isOneLineExpressionExport(body) {
+  if (body.length > 1) {
+    return false;
+  }
+
+  if (body[0].type !== "ExpressionStatement") {
+    return false;
+  }
+
+  if (body[0].expression.type !== "AssignmentExpression") {
+    return false;
+  }
+
+  return exportAssignmentHasRequireLeave(body[0].expression.right);
+}
+
+export function exportAssignmentHasRequireLeave(expr) {
+  if (expr.type === "LogicalExpression") {
+    return atLeastOneBranchHasRequireLeave(expr.left, expr.right);
+  }
+
+  if (expr.type === "ConditionalExpression") {
+    return atLeastOneBranchHasRequireLeave(expr.consequent, expr.alternate);
+  }
+
+  if (expr.type === "CallExpression") {
+    return getCallExpressionIdentifier(expr) === "require";
+  }
+
+  if (expr.type === "MemberExpression") {
+    let rootMember = expr.object;
+    while (rootMember.type === "MemberExpression") {
+      rootMember = rootMember.object;
+    }
+
+    if (rootMember.type !== "CallExpression") {
+      return false;
+    }
+
+    return getCallExpressionIdentifier(rootMember) === "require";
+  }
+
+  return false;
+}
+
+function atLeastOneBranchHasRequireLeave(left, right) {
+  return [
+    exportAssignmentHasRequireLeave(left),
+    exportAssignmentHasRequireLeave(right)
+  ].some((hasRequire) => hasRequire);
+}
+
 export function rootLocation() {
   return { start: { line: 0, column: 0 }, end: { line: 0, column: 0 } };
 }

test/issues/170-isOneLineRequire-logicalExpression-CJS-export.spec.js

@@ -0,0 +1,68 @@
+// Import Node.js Dependencies
+import { test } from "node:test";
+import assert from "node:assert";
+
+// Import Internal Dependencies
+import { runASTAnalysis } from "../../index.js";
+
+const validTestCases = [
+  ["module.exports = require('fs') || require('constants');", ["fs", "constants"]],
+  ["module.exports = require('constants') ? require('fs') : require('foo');", ["constants", "fs", "foo"]],
+
+  // should have at least one branch has a `require` callee
+  ["module.exports = require('constants') || {};", ["constants"]],
+  ["module.exports = {} || require('constants');", ["constants"]],
+  ["module.exports = require('constants') ? require('fs') : {};", ["constants", "fs"]],
+  ["module.exports = require('constants') ? {} : require('fs');", ["constants", "fs"]],
+
+  // should apply to nested conditions
+  ["module.exports = (require('constants') || {}) || (require('foo') || {});", ["constants", "foo"]],
+  ["module.exports = require('constants') ? (require('fs') || {}) : ({} || require('foo'));", ["constants", "fs", "foo"]],
+  ["module.exports = require('constants') ? ({} || require('fs')) : (require('foo') || {});", ["constants", "fs", "foo"]],
+  ["module.exports = require('constants') ? (require('fs') ? {} : require('bar')) : {};", ["constants", "fs", "bar"]],
+  ["module.exports = require('constants') ? {} : (require('fs') ? {} : require('bar'));", ["constants", "fs", "bar"]],
+
+  // test condition that are not `require` callees, here `notRequire('someModule')`, are ignored
+  ["module.exports = notRequire('someModule') ? require('constants') : require('foo');",
+    ["constants", "foo"]
+  ],
+  ["module.exports = ok ? (notRequire('someModule') ? require('constants') : require('foo')) : {};",
+    ["constants", "foo"]
+  ],
+  ["module.exports = ok ? {} : (notRequire('someModule') ? require('constants') : require('foo'));",
+    ["constants", "foo"]
+  ]
+];
+
+test("it should return isOneLineRequire true given a single line CJS export with a valid assignment", () => {
+  validTestCases.forEach((test) => {
+    const [source, modules] = test;
+    const { dependencies, isOneLineRequire } = runASTAnalysis(source);
+
+    assert.ok(isOneLineRequire);
+    assert.deepEqual([...dependencies.keys()], modules);
+  });
+});
+
+const invalidTestCases = [
+  // should have at least one `require` callee
+  ["module.exports = notRequire('foo') || {};", []],
+  ["module.exports = {} || notRequire('foo');", []],
+  ["module.exports = require('constants') ? {} : {};", ["constants"]],
+
+  // same behavior should apply to nested conditions
+  ["module.exports = (notRequire('foo') || {}) || (notRequire('foo') || {});", []],
+  ["module.exports = require('constants') ? (notRequire('foo') || {}) : (notRequire('foo') || {});", ["constants"]],
+  ["module.exports = require('constants') ? (notRequire('foo') || {}) : (notRequire('foo') || {});", ["constants"]],
+  ["module.exports = require('constants') ? (require('constants') ? {} : {}) : (require('constants') ? {} : {});", ["constants"]]
+];
+
+test("it should return isOneLineRequire false given a single line CJS export with illegal callees", () => {
+  invalidTestCases.forEach((test) => {
+    const [source, modules] = test;
+    const { dependencies, isOneLineRequire } = runASTAnalysis(source);
+
+    assert.ok(isOneLineRequire === false);
+    assert.deepEqual([...dependencies.keys()], modules);
+  });
+});