refactor(ProjectBuilder): clean up output file collection code (#1099)

* refactor(ProjectBuilder): less repetitive fileSorter This reverts the fileSorter to the state from before #937, but using our own simple re-implementation of `compare-func`. * fix(ProjectBuilder): apply sort RegExp to basename only * refactor(ProjectBuilder): use fast-glob instead of hand-rolled equivalent * refactor(ProjectBuilder): factor out common isPathArchSpecific * refactor(ProjectBuilder): use includes instead of indexOf * refactor(ProjectBuilder): move sorting into findOutputFilesHelper * refactor(ProjectBuilder): simplify findOutputFiles signature
apache · Nov 21, 2020 · b245337 · b245337
1 parent bb7d733
commit b245337
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 64 deletions.
diff --git a/bin/templates/cordova/lib/builders/ProjectBuilder.js b/bin/templates/cordova/lib/builders/ProjectBuilder.js
@@ -20,10 +20,12 @@
 var fs = require('fs-extra');
 var path = require('path');
 const execa = require('execa');
+const glob = require('fast-glob');
 var events = require('cordova-common').events;
 var CordovaError = require('cordova-common').CordovaError;
 var check_reqs = require('../check_reqs');
 var PackageType = require('../PackageType');
+const { compareByAll } = require('../utils');
 const { createEditor } = require('properties-parser');
 
 const MARKER = 'YOUR CHANGES WILL BE ERASED!';
@@ -32,83 +34,46 @@ const TEMPLATE =
     '# This file is automatically generated.\n' +
     '# Do not modify this file -- ' + MARKER + '\n';
 
-const archSpecificRegex = /-x86|-arm/;
-const unsignedBuildRegex = /-unsigned/;
+const isPathArchSpecific = p => /-x86|-arm/.test(path.basename(p));
 
-const fileSorter = (filePathA, filePathB) => {
-    const archSpecificA = archSpecificRegex.test(filePathA);
-    const archSpecificB = archSpecificRegex.test(filePathB);
+const outputFileComparator = compareByAll([
+    // Sort arch specific builds after generic ones
+    isPathArchSpecific,
 
-    // If they are not equal, then sort by specific archs after generic ones
-    if (archSpecificA !== archSpecificB) {
-        return archSpecificA < archSpecificB ? -1 : 1;
-    }
-
-    // Otherwise, move onto the next sort item, which is by sorting unsigned bulds after signed ones
-    const unsignedA = unsignedBuildRegex.test(filePathA);
-    const unsignedB = unsignedBuildRegex.test(filePathB);
-
-    if (unsignedA !== unsignedB) {
-        return unsignedA < unsignedB ? -1 : 1;
-    }
-
-    // Then, sort by modification time, latest first
-    const modTimeA = fs.statSync(filePathA).mtime.getTime();
-    const modTimeB = fs.statSync(filePathB).mtime.getTime();
-
-    if (modTimeA !== modTimeB) {
-        return modTimeA < modTimeB ? 1 : -1;
-    }
+    // Sort unsigned builds after signed ones
+    filePath => /-unsigned/.test(path.basename(filePath)),
 
-    // Finally, if all above is the same, sort by file name length, ascending
-    return filePathB.length < filePathA.length ? -1 : 1;
-};
+    // Sort by file modification time, latest first
+    filePath => -fs.statSync(filePath).mtime.getTime(),
 
-/**
- * If the provided directory does not exist or extension is missing, return an empty array.
- * If the director exists, loop the directories and collect list of files matching the extension.
- *
- * @param {String} dir Directory to scan
- * @param {String} extension
- */
-function recursivelyFindFiles (dir, extension) {
-    if (!fs.existsSync(dir) || !extension) return [];
-
-    const files = fs.readdirSync(dir, { withFileTypes: true })
-        .map(entry => {
-            const item = path.resolve(dir, entry.name);
-
-            if (entry.isDirectory()) return recursivelyFindFiles(item, extension);
-            if (path.extname(entry.name) === `.${extension}`) return item;
-            return false;
-        });
-
-    return Array.prototype.concat(...files)
-        .filter(file => file !== false);
-}
+    // Sort by file name length, ascending
+    filePath => filePath.length
+]);
 
 /**
- * @param {String} dir
- * @param {String} build_type
- * @param {String} arch
- * @param {String} extension
+ * @param {'apk' | 'aab'} bundleType
+ * @param {'debug' | 'release'} buildType
+ * @param {{arch?: string}} options
  */
-function findOutputFilesHelper (dir, build_type, arch, extension) {
-    let files = recursivelyFindFiles(path.resolve(dir, build_type), extension);
+function findOutputFiles (bundleType, buildType, { arch }) {
+    let files = glob.sync(`**/*.${bundleType}`, {
+        absolute: true,
+        cwd: path.resolve(this[`${bundleType}Dir`], buildType)
+    }).map(path.normalize);
 
     if (files.length === 0) return files;
 
     // Assume arch-specific build if newest apk has -x86 or -arm.
-    const archSpecific = !!/-x86|-arm/.exec(path.basename(files[0]));
+    const archSpecific = isPathArchSpecific(files[0]);
 
     // And show only arch-specific ones (or non-arch-specific)
-    files = files.filter(p => !!/-x86|-arm/.exec(path.basename(p)) === archSpecific);
+    files = files.filter(p => isPathArchSpecific(p) === archSpecific);
 
     if (archSpecific && files.length > 1 && arch) {
-        files = files.filter(p => path.basename(p).indexOf('-' + arch) !== -1);
+        files = files.filter(p => path.basename(p).includes('-' + arch));
     }
 
-    return files;
+    return files.sort(outputFileComparator);
 }
 
 class ProjectBuilder {
@@ -362,11 +327,11 @@ class ProjectBuilder {
     }
 
     findOutputApks (build_type, arch) {
-        return findOutputFilesHelper(this.apkDir, build_type, arch, 'apk').sort(fileSorter);
+        return findOutputFiles.call(this, 'apk', build_type, { arch });
     }
 
     findOutputBundles (build_type) {
-        return findOutputFilesHelper(this.aabDir, build_type, false, 'aab').sort(fileSorter);
+        return findOutputFiles.call(this, 'aab', build_type);
     }
 
     fetchBuildResults (build_type, arch) {

diff --git a/bin/templates/cordova/lib/utils.js b/bin/templates/cordova/lib/utils.js
@@ -39,3 +39,17 @@ exports.replaceFileContents = function (file, searchRegex, replacementString) {
     contents = contents.replace(searchRegex, replacementString);
     fs.writeFileSync(file, contents);
 };
+
+// Some helpers for easier sorting
+exports.compare = (a, b) => (a < b && -1) || +(a > b);
+exports.compareBy = f => (a, b) => exports.compare(f(a), f(b));
+exports.compareByAll = fns => {
+    const comparators = fns.map(exports.compareBy);
+    return (a, b) => {
+        for (const cmp of comparators) {
+            const result = cmp(a, b);
+            if (result) return result;
+        }
+        return 0;
+    };
+};
diff --git a/spec/unit/builders/ProjectBuilder.spec.js b/spec/unit/builders/ProjectBuilder.spec.js
@@ -289,7 +289,7 @@ describe('ProjectBuilder', () => {
         });
     });
 
-    describe('fileSorter', () => {
+    describe('outputFileComparator', () => {
         it('should sort APKs from most recent to oldest, deprioritising unsigned arch-specific builds', () => {
             const APKs = {
                 'app-debug.apk': new Date('2018-04-20'),
@@ -309,7 +309,7 @@ describe('ProjectBuilder', () => {
             });
 
             const apkArray = Object.keys(APKs);
-            const sortedApks = apkArray.sort(ProjectBuilder.__get__('fileSorter'));
+            const sortedApks = apkArray.sort(ProjectBuilder.__get__('outputFileComparator'));
 
             expect(sortedApks).toEqual(expectedResult);
         });