From b8185d48a75e65932196700e28bf71613dd141b4 Mon Sep 17 00:00:00 2001 From: beyondkmp Date: Mon, 7 Oct 2024 12:23:23 +0800 Subject: [PATCH] fix: support including node_modules in other subdirectories (#8562) fix /~https://github.com/electron-userland/electron-builder/pull/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 --- .changeset/tricky-files-speak.md | 6 + packages/app-builder-lib/scheme.json | 5 - packages/app-builder-lib/src/configuration.ts | 6 - packages/app-builder-lib/src/fileMatcher.ts | 14 +- .../app-builder-lib/src/util/AppFileWalker.ts | 33 +---- packages/app-builder-lib/src/util/filter.ts | 10 +- packages/builder-util/src/fs.ts | 4 +- test/snapshots/ignoreTest.js.snap | 12 -- test/src/ignoreTest.ts | 127 ++++++++++++++---- 9 files changed, 131 insertions(+), 86 deletions(-) create mode 100644 .changeset/tricky-files-speak.md diff --git a/.changeset/tricky-files-speak.md b/.changeset/tricky-files-speak.md new file mode 100644 index 00000000000..c34cb65990a --- /dev/null +++ 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 index d283ab2af60..fdce744dd4a 100644 --- 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 index 86a2e2c9af1..2ca16a8c4ac 100644 --- 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 index 5afa4220f90..32affd2ea19 100644 --- 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 index 579f8633152..dc2b9952ebd 100644 --- 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): 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 index 51fe2e515d5..f0710cad5fe 100644 --- 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, 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 index b00bbf51e9d..32d13dc2b17 100644 --- 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 index c2543ed1d83..299ed97f063 100644 --- 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 index 1cb08a560c3..c9facb29121 100644 --- a/test/src/ignoreTest.ts +++ b/test/src/ignoreTest.ts @@ -83,24 +83,24 @@ 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(), ]) }, } @@ -108,33 +108,38 @@ test.ifNotCiMac( ) 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(), ]) }, } @@ -142,13 +147,12 @@ test.ifDevOrLinuxCi( ) 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, }, }, { @@ -156,33 +160,40 @@ test.ifDevOrLinuxCi( 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/**"], }, }, { @@ -190,24 +201,82 @@ test.skip.ifDevOrLinuxCi( 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(), + ]) + }, + } + ) +) \ No newline at end of file