fix: support including node_modules in other subdirectories (#8562)

fix #6080 and support including node_modules in other subdirectories **How to fix** 1. No file patterns - The final File patterns are `["**/*", "!**/node_modules/**", "!dist{,/**/*}", ...] ` 2. File patterns with other sub node_modules `["/*", "**/sub/node_modules/**"]` - File patterns are `["**/*", "!**/node_modules/**", "**/sub/node_modules/**", "!dist{,/**/*}", ...]` 3. File patterns without sub node_modules - The final File patterns are `["**/*", "!**/node_modules/**", "!dist{,/**/*}", ...]` The final patterns above all filter out the node_modules in the app root directory. In filter.ts, we handle this by returning false by default if relative === 'node_modules', so it won't be filtered out. /~https://github.com/electron-userland/electron-builder/blob/e2c79819751454dbd1a939610d66e940b5dfb73d/packages/app-builder-lib/src/util/filter.ts#L60-L62 However, if you really want to filter out the node_modules in the app directory, you can use `["!node_modules/**/*"]` to filter it. **Two points to note** 1. Now `["**/*", "**/sub/node_modules"]` cannot match. Only `["**/*", "**/sub/node_modules/**"]` will match. For a relative path of` sub/node_modules`, if don't add a `/ ` at the end, we can only use `**/sub/node_module` to match. If add a `/` at the end, we can only use `**/sub/node_module/**` to match. **I currently prefer that `**/sub/node_modules/**` matches, as this aligns more with conventional usage.** 2. `*/sub/node_modules/**` cannot match because the relative path is `sub/node_modules`, and there's no extra directory at the beginning. --------- Co-authored-by: beyondkmp <beyondkmkp@gmail.com>
electron-userland · Oct 7, 2024 · b8185d4 · b8185d4
1 parent 13f55a3
commit b8185d4
Show file tree

Hide file tree

Showing 9 changed files with 131 additions and 86 deletions.
diff --git a/.changeset/tricky-files-speak.md b/.changeset/tricky-files-speak.md
@@ -0,0 +1,6 @@
+---
+"app-builder-lib": major 
+"builder-util": major 
+---
+
+support including node_modules in other subdirectories
diff --git a/packages/app-builder-lib/scheme.json b/packages/app-builder-lib/scheme.json
@@ -7145,11 +7145,6 @@
       "description": "Whether to include PDB files.",
       "type": "boolean"
     },
-    "includeSubNodeModules": {
-      "default": false,
-      "description": "Whether to include *all* of the submodules node_modules directories",
-      "type": "boolean"
-    },
     "launchUiVersion": {
       "description": "*libui-based frameworks only* The version of LaunchUI you are packaging for. Applicable for Windows only. Defaults to version suitable for used framework version.",
       "type": [

diff --git a/packages/app-builder-lib/src/configuration.ts b/packages/app-builder-lib/src/configuration.ts
@@ -183,12 +183,6 @@ export interface CommonConfiguration {
   readonly removePackageKeywords?: boolean
 }
 export interface Configuration extends CommonConfiguration, PlatformSpecificBuildOptions, Hooks {
-  /**
-   * Whether to include *all* of the submodules node_modules directories
-   * @default false
-   */
-  includeSubNodeModules?: boolean
-
   /**
    * Whether to use [electron-compile](http://github.com/electron/electron-compile) to compile app. Defaults to `true` if `electron-compile` in the dependencies. And `false` if in the `devDependencies` or doesn't specified.
    */

diff --git a/packages/app-builder-lib/src/fileMatcher.ts b/packages/app-builder-lib/src/fileMatcher.ts
@@ -163,7 +163,19 @@ export function getMainFileMatchers(
     patterns.push("package.json")
   }
 
-  customFirstPatterns.push("!**/node_modules")
+  let insertExculdeNodeModulesIndex = -1
+  for (let i = 0; i < patterns.length; i++) {
+    if (!patterns[i].startsWith("!") && (patterns[i].includes("/node_modules") || patterns[i].includes("node_modules/"))) {
+      insertExculdeNodeModulesIndex = i
+      break
+    }
+  }
+
+  if (insertExculdeNodeModulesIndex !== -1) {
+    patterns.splice(insertExculdeNodeModulesIndex, 0, ...["!**/node_modules/**"])
+  } else {
+    customFirstPatterns.push("!**/node_modules/**")
+  }
 
   // /~https://github.com/electron-userland/electron-builder/issues/1482
   const relativeBuildResourceDir = path.relative(matcher.from, buildResourceDir)

diff --git a/packages/app-builder-lib/src/util/AppFileWalker.ts b/packages/app-builder-lib/src/util/AppFileWalker.ts
@@ -4,8 +4,6 @@ import { FileMatcher } from "../fileMatcher"
 import { Packager } from "../packager"
 import * as path from "path"
 
-const nodeModulesSystemDependentSuffix = `${path.sep}node_modules`
-
 function addAllPatternIfNeed(matcher: FileMatcher) {
   if (!matcher.isSpecifiedAsEmptyArray && (matcher.isEmpty() || matcher.containsOnlyIgnore())) {
     matcher.prependPattern("**/*")
@@ -55,22 +53,7 @@ function createAppFilter(matcher: FileMatcher, packager: Packager): Filter | nul
   if (packager.areNodeModulesHandledExternally) {
     return matcher.isEmpty() ? null : matcher.createFilter()
   }
-
-  const nodeModulesFilter: Filter = (file, fileStat) => {
-    return !(fileStat.isDirectory() && file.endsWith(nodeModulesSystemDependentSuffix))
-  }
-
-  if (matcher.isEmpty()) {
-    return nodeModulesFilter
-  }
-
-  const filter = matcher.createFilter()
-  return (file, fileStat) => {
-    if (!nodeModulesFilter(file, fileStat)) {
-      return !!packager.config.includeSubNodeModules
-    }
-    return filter(file, fileStat)
-  }
+  return matcher.createFilter()
 }
 
 /** @internal */
@@ -85,18 +68,8 @@ export class AppFileWalker extends FileCopyHelper implements FileConsumer {
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   consume(file: string, fileStat: Stats, parent: string, siblingNames: Array<string>): any {
     if (fileStat.isDirectory()) {
-      // /~https://github.com/electron-userland/electron-builder/issues/1539
-      // but do not filter if we inside node_modules dir
-      // update: solution disabled, node module resolver should support such setup
-      if (file.endsWith(nodeModulesSystemDependentSuffix)) {
-        if (!this.packager.config.includeSubNodeModules) {
-          const matchesFilter = this.matcherFilter(file, fileStat)
-          if (!matchesFilter) {
-            // Skip the file
-            return false
-          }
-        }
-      }
+      const matchesFilter = this.matcherFilter(file, fileStat)
+      return !matchesFilter
     } else {
       // save memory - no need to store stat for directory
       this.metadata.set(file, fileStat)

diff --git a/packages/app-builder-lib/src/util/filter.ts b/packages/app-builder-lib/src/util/filter.ts
@@ -54,7 +54,15 @@ export function createFilter(src: string, patterns: Array<Minimatch>, excludePat
       return true
     }
 
-    const relative = getRelativePath(file, srcWithEndSlash)
+    let relative = getRelativePath(file, srcWithEndSlash)
+
+    // filter the root node_modules, but not a subnode_modules (like /appDir/others/foo/node_modules/blah)
+    if (relative === "node_modules") {
+      return false
+    } else if (relative.endsWith("/node_modules")) {
+      relative += "/"
+    }
+
     // /~https://github.com/electron-userland/electron-builder/issues/867
     return minimatchAll(relative, patterns, stat) && (excludePatterns == null || stat.isDirectory() || !minimatchAll(relative, excludePatterns, stat))
   }

diff --git a/packages/builder-util/src/fs.ts b/packages/builder-util/src/fs.ts
@@ -88,9 +88,9 @@ export async function walk(initialDirPath: string, filter?: Filter | null, consu
           }
 
           const consumerResult = consumer == null ? null : consumer.consume(filePath, stat, dirPath, childNames)
-          if (consumerResult === false) {
+          if (consumerResult === true) {
             return null
-          } else if (consumerResult == null || !("then" in consumerResult)) {
+          } else if (consumerResult === false || consumerResult == null || !("then" in consumerResult)) {
             if (stat.isDirectory()) {
               dirs.push(name)
               return null

diff --git a/test/snapshots/ignoreTest.js.snap b/test/snapshots/ignoreTest.js.snap
@@ -6,18 +6,6 @@ Object {
 }
 `;
 
-exports[`copied all submodule node_modules 1`] = `
-Object {
-  "linux": Array [],
-}
-`;
-
-exports[`copied no submodule node_modules 1`] = `
-Object {
-  "linux": Array [],
-}
-`;
-
 exports[`copied select submodule node_modules 1`] = `
 Object {
   "linux": Array [],

diff --git a/test/src/ignoreTest.ts b/test/src/ignoreTest.ts
@@ -83,131 +83,200 @@ test.ifNotCiMac(
       targets: Platform.LINUX.createTarget(DIR_TARGET),
       config: {
         asar: false,
+        files: ["**/*", "**/submodule-1-test/node_modules/**"],
       },
     },
     {
+      isInstallDepsBefore: true,
       projectDirCreated: projectDir => {
         return Promise.all([
           modifyPackageJson(projectDir, data => {
             data.devDependencies = {
-              "@electron/osx-sign": "*",
+              "semver": "6.3.1",
               ...data.devDependencies,
             }
           }),
-          outputFile(path.join(projectDir, "node_modules", "@electron/osx-sign", "package.json"), "{}"),
         ])
       },
       packed: context => {
         return Promise.all([
-          assertThat(path.join(context.getResources(Platform.LINUX), "app", "node_modules", "@electron/osx-sign")).doesNotExist(),
-          assertThat(path.join(context.getResources(Platform.LINUX), "app", "ignoreMe")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX), "app", "node_modules", "semver")).doesNotExist(),
         ])
       },
     }
   )
 )
 
 test.ifDevOrLinuxCi(
-  "copied no submodule node_modules",
+  "copied sub node_modules of the rootDir/node_modules",
   app(
     {
       targets: Platform.LINUX.createTarget(DIR_TARGET),
       config: {
         asar: false,
-        includeSubNodeModules: false,
+        files: ["**/*", "**/submodule-1-test/node_modules/**"],
       },
     },
     {
+      isInstallDepsBefore: true,
       projectDirCreated: projectDir => {
         return Promise.all([
           modifyPackageJson(projectDir, data => {
             data.dependencies = {
-              "submodule-1-test": "*",
-              "submodule-2-test": "*",
+              "electron-updater": "6.3.9",
+              "semver":"6.3.1",
               ...data.dependencies,
             }
           }),
-          outputFile(path.join(projectDir, "node_modules", "submodule-1-test", "node_modules", "package.json"), "{}"),
-          outputFile(path.join(projectDir, "node_modules", "submodule-2-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "submodule-1-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "others", "node_modules", "package.json"), "{}"),
         ])
       },
       packed: context => {
         return Promise.all([
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-1-test", "node_modules")).doesNotExist(),
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-2-test", "node_modules")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "electron-updater", "node_modules")).isDirectory(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "node_modules")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules")).isDirectory(),
+          assertThat(
+            path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules", "package.json")
+          ).isFile(),
         ])
       },
     }
   )
 )
 
 test.ifDevOrLinuxCi(
-  "copied all submodule node_modules",
+  "Don't copy sub node_modules of the other dir instead of rootDir",
   app(
     {
       targets: Platform.LINUX.createTarget(DIR_TARGET),
       config: {
         asar: false,
-        includeSubNodeModules: true,
       },
     },
     {
       projectDirCreated: projectDir => {
         return Promise.all([
           modifyPackageJson(projectDir, data => {
             data.dependencies = {
-              "submodule-1-test": "*",
-              "submodule-2-test": "*",
               ...data.dependencies,
             }
           }),
-          outputFile(path.join(projectDir, "node_modules", "submodule-1-test", "node_modules", "package.json"), "{}"),
-          outputFile(path.join(projectDir, "node_modules", "submodule-2-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "others", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "others", "test1", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "others", "submodule-2-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "others", "submodule-2-test", "test2", "package.json"), "{}"),
         ])
       },
       packed: context => {
         return Promise.all([
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-1-test", "node_modules")).isDirectory(),
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-2-test", "node_modules")).isDirectory(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "node_modules")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "test1")).isDirectory(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "test1", "package.json")).isFile(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "submodule-2-test", "node_modules")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "submodule-2-test", "test2")).isDirectory(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "others", "submodule-2-test", "test2", "package.json")).isFile(),
         ])
       },
     }
   )
 )
 
-test.skip.ifDevOrLinuxCi(
+test.ifDevOrLinuxCi(
   "copied select submodule node_modules",
   app(
     {
       targets: Platform.LINUX.createTarget(DIR_TARGET),
       config: {
         asar: false,
-        files: ["**/*", "*/submodule-1-test/node_modules/**"],
+        // should use **/ instead of */, 
+        // we use the related path to match, so the relative path is submodule-1-test/node_modules
+        // */ will not match submodule-1-test/node_modules 
+        files: ["**/*", "**/submodule-1-test/node_modules/**"],
       },
     },
     {
       projectDirCreated: projectDir => {
         return Promise.all([
           modifyPackageJson(projectDir, data => {
             data.dependencies = {
-              "submodule-1-test": "*",
-              "submodule-2-test": "*",
               ...data.dependencies,
             }
           }),
-          outputFile(path.join(projectDir, "node_modules", "submodule-1-test", "node_modules", "package.json"), "{}"),
-          outputFile(path.join(projectDir, "node_modules", "submodule-2-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "submodule-1-test", "node_modules", "package.json"), "{}"),
+          outputFile(path.join(projectDir, "submodule-2-test", "node_modules", "package.json"), "{}"),
         ])
       },
       packed: context => {
         return Promise.all([
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-1-test", "node_modules")).isDirectory(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules")).isDirectory(),
           assertThat(
-            path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-1-test", "node_modules", "package.json")
+            path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules", "package.json")
           ).isFile(),
-          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "node_modules", "submodule-2-test", "node_modules")).doesNotExist(),
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-2-test", "node_modules")).doesNotExist(),
+        ])
+      },
+    }
+  )
+)
+
+test.ifDevOrLinuxCi(
+  "cannot copied select submodule node_modules by */",
+  app(
+    {
+      targets: Platform.LINUX.createTarget(DIR_TARGET),
+      config: {
+        asar: false,
+        files: ["**/*", "*/submodule-1-test/node_modules/**"],
+      },
+    },
+    {
+      projectDirCreated: projectDir => {
+        return Promise.all([
+          modifyPackageJson(projectDir, data => {
+            data.dependencies = {
+              ...data.dependencies,
+            }
+          }),
+          outputFile(path.join(projectDir, "submodule-1-test", "node_modules", "package.json"), "{}"),
+        ])
+      },
+      packed: context => {
+        return Promise.all([
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules")).doesNotExist(),
         ])
       },
     }
   )
 )
+
+test.ifDevOrLinuxCi(
+  "cannot copied select submodule node_modules by **/submodule-1-test/node_modules",
+  app(
+    {
+      targets: Platform.LINUX.createTarget(DIR_TARGET),
+      config: {
+        asar: false,
+        files: ["**/*", "**/submodule-1-test/node_modules"],
+      },
+    },
+    {
+      projectDirCreated: projectDir => {
+        return Promise.all([
+          modifyPackageJson(projectDir, data => {
+            data.dependencies = {
+              ...data.dependencies,
+            }
+          }),
+          outputFile(path.join(projectDir, "submodule-1-test", "node_modules", "package.json"), "{}"),
+        ])
+      },
+      packed: context => {
+        return Promise.all([
+          assertThat(path.join(context.getResources(Platform.LINUX, archFromString(process.arch)), "app", "submodule-1-test", "node_modules")).doesNotExist(),
+        ])
+      },
+    }
+  )
+)