From dff1fb21783714f40664a6ee2997aaec363043d9 Mon Sep 17 00:00:00 2001 From: Mark Lee Date: Sun, 7 Jan 2018 21:46:02 -0800 Subject: [PATCH] Refactor pruning to be part of the copy filter --- NEWS.md | 11 +++ common.js | 6 ++ docs/api.md | 17 +--- ignore.js | 19 ++-- package.json | 9 +- platform.js | 31 ++++--- prune.js | 86 ++++++++++++------- test/_setup.js | 1 + test/ci/before_install.sh | 8 +- test/fixtures/basic/package.json | 1 + .../electron-in-dependencies/index.html | 4 + .../fixtures/electron-in-dependencies/main.js | 1 + .../electron-in-dependencies/package.json | 8 ++ test/hooks.js | 50 ++++++----- test/prune.js | 86 ++++++------------- usage.txt | 3 - 16 files changed, 178 insertions(+), 163 deletions(-) create mode 100644 test/fixtures/electron-in-dependencies/index.html create mode 100644 test/fixtures/electron-in-dependencies/main.js create mode 100644 test/fixtures/electron-in-dependencies/package.json diff --git a/NEWS.md b/NEWS.md index e330cb99..68c6cb31 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,17 @@ [Unreleased]: https://github.com/electron-userland/electron-packager/compare/v11.2.0...master +### Changed + +* `prune` exclusively utilizes the `galactus` module for pruning devDependencies, instead of + depending on package managers +* `electron-packager` is no longer ignored by default +* A warning is emitted when an Electron module is a production dependency + +### Removed + +* `packageManager` option + ## [11.2.0] - 2018-03-24 [11.2.0]: https://github.com/electron-userland/electron-packager/compare/v11.1.0...v11.2.0 diff --git a/common.js b/common.js index baca3c2e..156751b4 100644 --- a/common.js +++ b/common.js @@ -142,6 +142,12 @@ module.exports = { generateFinalBasename: generateFinalBasename, generateFinalPath: generateFinalPath, sanitizeAppName: sanitizeAppName, + /** + * Convert slashes to UNIX-format separators. + */ + normalizePath: function normalizePath (pathToNormalize) { + return pathToNormalize.replace(/\\/g, '/') + }, info: info, warning: warning diff --git a/docs/api.md b/docs/api.md index 6e8723f0..143703d1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -313,21 +313,6 @@ The base directory where the finished package(s) are created. Whether to replace an already existing output directory for a given platform (`true`) or skip recreating it (`false`). -##### `packageManager` - -*String* or *Boolean* (default: `npm`) - -The package manager used to [prune](#prune) `devDependencies` modules from the outputted Electron -app. Supported package managers: - -* [`npm`](https://npmjs.com/) -* [`cnpm`](https://github.com/cnpm/cnpm) (Does not currently work with Windows, see - [GitHub issue](https://github.com/electron-userland/electron-packager/issues/515#issuecomment-297604044)) -* [`yarn`](https://yarnpkg.com/) - -If set to `false` we will use a custom [pruning module](https://www.npmjs.com/package/pruner) to walk your dependency -tree and prune your dependencies for you. - ##### `platform` *String* (default: the arch of the host computer running Node) @@ -345,7 +330,7 @@ is not restricted to the official set if [`download.mirror`](#download) is set. *Boolean* (default: `true`) -Runs the [package manager](#packagemanager) command to remove all of the packages specified in the +Walks the `node_modules` dependency tree to remove all of the packages specified in the `devDependencies` section of `package.json` from the outputted Electron app. ##### `quiet` diff --git a/ignore.js b/ignore.js index 3b5a82cf..72786b89 100644 --- a/ignore.js +++ b/ignore.js @@ -3,12 +3,10 @@ const common = require('./common') const debug = require('debug')('electron-packager') const path = require('path') +const prune = require('./prune') const targets = require('./targets') const DEFAULT_IGNORES = [ - '/node_modules/electron($|/)', - '/node_modules/electron-prebuilt(-compile)?($|/)', - '/node_modules/electron-packager($|/)', '/\\.git($|/)', '/node_modules/\\.bin($|/)', '\\.o(bj)?$' @@ -30,8 +28,8 @@ function generateIgnores (opts) { } function generateOutIgnores (opts) { - let normalizedOut = opts.out ? path.resolve(opts.out) : null - let outIgnores = [] + const normalizedOut = opts.out ? path.resolve(opts.out) : null + const outIgnores = [] if (normalizedOut === null || normalizedOut === process.cwd()) { for (const platform of Object.keys(targets.officialPlatformArchCombos)) { for (const arch of targets.officialPlatformArchCombos[platform]) { @@ -66,7 +64,8 @@ function userIgnoreFilter (opts) { } } - let outIgnores = generateOutIgnores(opts) + const outIgnores = generateOutIgnores(opts) + const pruner = opts.prune ? new prune.Pruner(opts.dir) : null return function filter (file) { if (outIgnores.indexOf(file) !== -1) { @@ -76,8 +75,12 @@ function userIgnoreFilter (opts) { let name = file.split(path.resolve(opts.dir))[1] if (path.sep === '\\') { - // convert slashes so unix-format ignores work - name = name.replace(/\\/g, '/') + name = common.normalizePath(name) + } + + if (pruner && name.startsWith('/node_modules/')) { + return prune.isModule(file) + .then(isModule => isModule ? pruner.pruneModule(name) : ignoreFunc(name)) } return ignoreFunc(name) diff --git a/package.json b/package.json index 60fe24f4..84ddf91c 100644 --- a/package.json +++ b/package.json @@ -23,13 +23,12 @@ "electron-osx-sign": "^0.4.1", "extract-zip": "^1.0.3", "fs-extra": "^5.0.0", + "galactus": "^0.2.0", "get-package-info": "^1.0.0", - "mz": "^2.6.0", "nodeify": "^1.0.1", "parse-author": "^2.0.0", "pify": "^3.0.0", "plist": "^2.0.0", - "pruner": "^0.0.7", "rcedit": "^1.0.0", "resolve": "^1.1.6", "sanitize-filename": "^1.6.0", @@ -40,18 +39,18 @@ "ava": "^0.25.0", "buffer-equal": "^1.0.0", "codecov": "^3.0.0", - "eslint": "^4.1.0", + "eslint": "^4.18.0", "eslint-config-standard": "^11.0.0", "eslint-plugin-ava": "^4.3.0", "eslint-plugin-import": "^2.2.0", "eslint-plugin-node": "^6.0.0", "eslint-plugin-promise": "^3.5.0", "eslint-plugin-standard": "^3.0.0", + "mz": "^2.6.0", "nyc": "^11.0.0", "pkg-up": "^2.0.0", "sinon": "^4.1.3", - "tempy": "^0.2.1", - "which": "^1.2.14" + "tempy": "^0.2.1" }, "engines": { "node": ">= 4.0" diff --git a/platform.js b/platform.js index 6da0f226..90233cf5 100644 --- a/platform.js +++ b/platform.js @@ -6,16 +6,18 @@ const fs = require('fs-extra') const path = require('path') const pify = require('pify') +const common = require('./common') const hooks = require('./hooks') const ignore = require('./ignore') -const pruneModules = require('./prune').pruneModules - -const common = require('./common') class App { constructor (opts, templatePath) { this.opts = opts this.templatePath = templatePath + + if (this.opts.prune === undefined) { + this.opts.prune = true + } } /** @@ -94,7 +96,6 @@ class App { }).then(() => fs.remove(path.join(this.originalResourcesDir, 'default_app.asar'))) // Prune and asar are performed before platform-specific logic, primarily so that // this.originalResourcesAppDir is predictable (e.g. before .app is renamed for mac) - .then(() => this.prune()) .then(() => this.asarApp()) } @@ -107,7 +108,18 @@ class App { this.opts.electronVersion, this.opts.platform, this.opts.arch - ])) + ])).then(() => { + if (this.opts.prune) { + return hooks.promisifyHooks(this.opts.afterPrune, [ + this.originalResourcesAppDir, + this.opts.electronVersion, + this.opts.platform, + this.opts.arch + ]) + } else { + return true + } + }) } /** @@ -130,15 +142,6 @@ class App { .catch(/* istanbul ignore next */ () => null) } - prune () { - if (this.opts.prune || this.opts.prune === undefined) { - return pruneModules(this.opts, this.originalResourcesAppDir) - .then(() => hooks.promisifyHooks(this.opts.afterPrune, [this.originalResourcesAppDir, this.opts.electronVersion, this.opts.platform, this.opts.arch])) - } - - return Promise.resolve() - } - asarApp () { const asarOptions = common.createAsarOpts(this.opts) if (!asarOptions) { diff --git a/prune.js b/prune.js index e9484b36..8f677a12 100644 --- a/prune.js +++ b/prune.js @@ -1,45 +1,69 @@ 'use strict' -const child = require('mz/child_process') -const debug = require('debug')('electron-packager') -const Walker = require('pruner').Walker - -const knownPackageManagers = ['npm', 'cnpm', 'yarn'] - -function pruneCommand (packageManager) { - switch (packageManager) { - case 'npm': - case 'cnpm': - return `${packageManager} prune --production` - case 'yarn': - return `${packageManager} install --production --no-bin-links --no-lockfile` +const common = require('./common') +const galactus = require('galactus') +const fs = require('fs-extra') +const path = require('path') + +const ELECTRON_MODULES = [ + 'electron', + 'electron-prebuilt', + 'electron-prebuilt-compile' +] + +class Pruner { + constructor (dir) { + this.baseDir = common.normalizePath(dir) + this.galactus = new galactus.DestroyerOfModules({ + rootDirectory: dir, + shouldKeepModuleTest: (module, isDevDep) => this.shouldKeepModule(module, isDevDep) + }) + this.walkedTree = false } -} -function pruneModules (opts, appPath) { - if (opts.packageManager === false) { - const walker = new Walker(appPath) - return walker.prune() - } else { - const packageManager = opts.packageManager || 'npm' + setModuleMap (moduleMap) { + this.moduleMap = new Map() + // destructured assignments are in Node 6 + for (const modulePair of moduleMap) { + const modulePath = modulePair[0] + const module = modulePair[1] + this.moduleMap.set(`/${common.normalizePath(modulePath)}`, module) + } + this.walkedTree = true + } - /* istanbul ignore if */ - if (packageManager === 'cnpm' && process.platform === 'win32') { - return Promise.reject(new Error('cnpm support does not currently work with Windows, see: https://github.com/electron-userland/electron-packager/issues/515#issuecomment-297604044')) + pruneModule (name) { + if (this.walkedTree) { + return this.isProductionModule(name) + } else { + return this.galactus.collectKeptModules({ relativePaths: true }) + .then(moduleMap => this.setModuleMap(moduleMap)) + .then(() => this.isProductionModule(name)) } + } - const command = pruneCommand(packageManager) + shouldKeepModule (module, isDevDep) { + if (isDevDep || module.depType === galactus.DepType.ROOT) { + return false + } - if (command) { - debug(`Pruning modules via: ${command}`) - return child.exec(command, { cwd: appPath }) - } else { - return Promise.reject(new Error(`Unknown package manager "${packageManager}". Known package managers: ${knownPackageManagers.join(', ')}`)) + // Node 6 has Array.prototype.includes + if (ELECTRON_MODULES.indexOf(module.name) !== -1) { + common.warning(`Found '${module.name}' but not as a devDependency, pruning anyway`) + return false } + + return true + } + + isProductionModule (name) { + return !!this.moduleMap.get(name) } } module.exports = { - pruneCommand: pruneCommand, - pruneModules: pruneModules + isModule: function isModule (pathToCheck) { + return fs.pathExists(path.join(pathToCheck, 'package.json')) + }, + Pruner: Pruner } diff --git a/test/_setup.js b/test/_setup.js index a9b2f6bf..2afeec93 100644 --- a/test/_setup.js +++ b/test/_setup.js @@ -76,6 +76,7 @@ function npmInstallForFixtures () { const fixtures = [ 'basic', 'basic-renamed-to-electron', + 'electron-in-dependencies', 'infer-missing-version-only', 'el-0374' ] diff --git a/test/ci/before_install.sh b/test/ci/before_install.sh index 6ee9068f..2ccd96b6 100755 --- a/test/ci/before_install.sh +++ b/test/ci/before_install.sh @@ -6,11 +6,9 @@ case "$TRAVIS_OS_NAME" in # Not using Trusty containers because it can't install wine1.6(-i386), # see: https://github.com/travis-ci/travis-ci/issues/6460 sudo rm /etc/apt/sources.list.d/google-chrome.list - sudo apt-key adv --fetch-keys http://dl.yarnpkg.com/debian/pubkey.gpg - echo "deb http://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list sudo dpkg --add-architecture i386 sudo apt-get update - sudo apt-get install -y wine1.6 yarn + sudo apt-get install -y wine1.6 ;; "osx") # Create CA @@ -31,9 +29,5 @@ case "$TRAVIS_OS_NAME" in npm install wine-darwin@1.9.17-1 # Setup ~/.wine by running a command ./node_modules/.bin/wine hostname - # Install yarn - npm install -g yarn ;; esac - -npm install -g cnpm diff --git a/test/fixtures/basic/package.json b/test/fixtures/basic/package.json index 5fe25727..ada9e8f6 100644 --- a/test/fixtures/basic/package.json +++ b/test/fixtures/basic/package.json @@ -3,6 +3,7 @@ "version": "4.99.101", "productName": "MainJS", "dependencies": { + "@types/node": "^8.0.0", "run-series": "^1.1.1" }, "//": "ncp used to test https://github.com/electron-userland/electron-packager/pull/186", diff --git a/test/fixtures/electron-in-dependencies/index.html b/test/fixtures/electron-in-dependencies/index.html new file mode 100644 index 00000000..c0617ee1 --- /dev/null +++ b/test/fixtures/electron-in-dependencies/index.html @@ -0,0 +1,4 @@ + + + Hello, world! + diff --git a/test/fixtures/electron-in-dependencies/main.js b/test/fixtures/electron-in-dependencies/main.js new file mode 100644 index 00000000..ccacec30 --- /dev/null +++ b/test/fixtures/electron-in-dependencies/main.js @@ -0,0 +1 @@ +'use strict' diff --git a/test/fixtures/electron-in-dependencies/package.json b/test/fixtures/electron-in-dependencies/package.json new file mode 100644 index 00000000..03598d73 --- /dev/null +++ b/test/fixtures/electron-in-dependencies/package.json @@ -0,0 +1,8 @@ +{ + "main": "main.js", + "productName": "MainJS", + "description": "Removing electron from dependencies", + "dependencies": { + "electron": "1.3.1" + } +} diff --git a/test/hooks.js b/test/hooks.js index 01604884..71f9eddf 100644 --- a/test/hooks.js +++ b/test/hooks.js @@ -6,29 +6,32 @@ const packager = require('..') const test = require('ava') const util = require('./_util') -function createHookTest (hookName) { - // 2 packages will be built during this test - util.packagerTest(`platform=all test (one arch) (${hookName} hook)`, (t, opts) => { - let hookCalled = false - opts.dir = util.fixtureSubdir('basic') - opts.electronVersion = config.version - opts.arch = 'ia32' - opts.platform = 'all' +function hookTest (wantHookCalled, hookName, t, opts) { + let hookCalled = false + opts.dir = util.fixtureSubdir('basic') + opts.electronVersion = config.version + opts.arch = 'ia32' + opts.platform = 'all' - opts[hookName] = [(buildPath, electronVersion, platform, arch, callback) => { - hookCalled = true - t.is(electronVersion, opts.electronVersion, `${hookName} electronVersion should be the same as the options object`) - t.is(arch, opts.arch, `${hookName} arch should be the same as the options object`) - callback() - }] + opts[hookName] = [(buildPath, electronVersion, platform, arch, callback) => { + hookCalled = true + t.is(electronVersion, opts.electronVersion, `${hookName} electronVersion should be the same as the options object`) + t.is(arch, opts.arch, `${hookName} arch should be the same as the options object`) + callback() + }] + + // 2 packages will be built during this test + return packager(opts) + .then(finalPaths => { + t.is(finalPaths.length, 2, 'packager call should resolve with expected number of paths') + t.is(wantHookCalled, hookCalled, `${hookName} methods ${wantHookCalled ? 'should' : 'should not'} have been called`) + return util.verifyPackageExistence(finalPaths) + }).then(exists => t.deepEqual(exists, [true, true], 'Packages should be generated for both 32-bit platforms')) +} - return packager(opts) - .then(finalPaths => { - t.is(finalPaths.length, 2, 'packager call should resolve with expected number of paths') - t.true(hookCalled, `${hookName} methods should have been called`) - return util.verifyPackageExistence(finalPaths) - }).then(exists => t.deepEqual(exists, [true, true], 'Packages should be generated for both 32-bit platforms')) - }) +function createHookTest (hookName) { + util.packagerTest(`platform=all test (one arch) (${hookName} hook)`, + (t, opts) => hookTest(true, hookName, t, opts)) } createHookTest('afterCopy') @@ -70,3 +73,8 @@ test('serialHooks executes functions serially', t => { return hooks.serialHooks(testHooks)(() => output) .then(result => t.is(result, '0 1 2 3 4 5 6 7 8 9 10', 'should be in sequential order')) }) + +util.packagerTest('prune hook does not get called when prune=false', (t, opts) => { + opts.prune = false + return hookTest(false, 'afterPrune', t, opts) +}) diff --git a/test/prune.js b/test/prune.js index 43c871e5..63f64601 100644 --- a/test/prune.js +++ b/test/prune.js @@ -2,14 +2,21 @@ const fs = require('fs-extra') const path = require('path') -const prune = require('../prune') -const test = require('ava') const util = require('./_util') -const which = require('which') -function assertYarnLockDoesntExist (t, resourcesPath) { - return fs.pathExists(path.join(resourcesPath, 'app', 'yarn.lock')) - .then(exists => t.false(exists, 'yarn.lock should NOT exist in packaged app')) +function checkDependency (t, resourcesPath, moduleName, moduleExists) { + const assertion = moduleExists ? 'should' : 'should NOT' + const message = `module dependency '${moduleName}' ${assertion} exist under app/node_modules` + const modulePath = path.join(resourcesPath, 'app', 'node_modules', moduleName) + return fs.pathExists(modulePath) + .then(exists => t.is(moduleExists, exists, message)) + .then(() => modulePath) +} + +function assertDependencyExists (t, resourcesPath, moduleName) { + return checkDependency(t, resourcesPath, moduleName, true) + .then(modulePath => fs.stat(modulePath)) + .then(stats => t.true(stats.isDirectory(), 'module is a directory')) } function createPruneOptionTest (t, baseOpts, prune, testMessage) { @@ -24,62 +31,25 @@ function createPruneOptionTest (t, baseOpts, prune, testMessage) { return util.packageAndEnsureResourcesPath(t, opts) .then(generatedResourcesPath => { resourcesPath = generatedResourcesPath - return fs.stat(path.join(resourcesPath, 'app', 'node_modules', 'run-series')) - }).then(stats => { - t.true(stats.isDirectory(), 'npm dependency should exist under app/node_modules') - return fs.pathExists(path.join(resourcesPath, 'app', 'node_modules', 'run-waterfall')) - }).then(exists => { - t.is(!prune, exists, testMessage) - if (opts.packageManager === 'yarn') { - return assertYarnLockDoesntExist(t, resourcesPath) - } - return Promise.resolve() - }) + return assertDependencyExists(t, resourcesPath, 'run-series') + }).then(() => assertDependencyExists(t, resourcesPath, '@types/node')) + .then(() => checkDependency(t, resourcesPath, 'run-waterfall', !prune)) + .then(() => checkDependency(t, resourcesPath, 'electron-prebuilt', !prune)) } -test('pruneCommand returns the correct command when passing a known package manager', t => { - t.is(prune.pruneCommand('npm'), 'npm prune --production', 'passing npm gives the npm prune command') - t.is(prune.pruneCommand('cnpm'), 'cnpm prune --production', 'passing cnpm gives the cnpm prune command') - t.is(prune.pruneCommand('yarn'), 'yarn install --production --no-bin-links --no-lockfile', 'passing yarn gives the yarn "prune command"') -}) - -test('pruneCommand returns undefined when the package manager is unknown', t => { - t.is(prune.pruneCommand('unknown-package-manager'), undefined) -}) - -test('pruneModules returns an error when the package manager is unknown', t => - t.throws(prune.pruneModules({packageManager: 'unknown-package-manager'}, '/tmp/app-path')) -) - -if (process.platform === 'win32') { - test('pruneModules returns an error when trying to use cnpm on Windows', t => - t.throws(prune.pruneModules({packageManager: 'cnpm'}, '/tmp/app-path')) - ) -} - -// This is not in the below loop so that it tests the default packageManager option. -util.testSinglePlatform('prune test with npm', (t, baseOpts) => { +util.testSinglePlatform('prune test', (t, baseOpts) => { return createPruneOptionTest(t, baseOpts, true, 'package.json devDependency should NOT exist under app/node_modules') }) -// Not in the loop because it doesn't depend on an executable -util.testSinglePlatform('prune test using pruner module (packageManager=false)', (t, baseOpts) => { - const opts = Object.assign({packageManager: false}, baseOpts) - return createPruneOptionTest(t, opts, true, 'package.json devDependency should NOT exist under app/node_modules') -}) +util.testSinglePlatform('prune electron in dependencies', (t, baseOpts) => { + const opts = Object.assign({}, baseOpts, { + name: 'pruneElectronTest', + dir: util.fixtureSubdir('electron-in-dependencies') + }) -for (const packageManager of ['cnpm', 'yarn']) { - try { - if (which.sync(packageManager)) { - util.testSinglePlatform(`prune test with ${packageManager}`, (t, baseOpts) => { - const opts = Object.assign({packageManager: packageManager}, baseOpts) - return createPruneOptionTest(t, opts, true, 'package.json devDependency should NOT exist under app/node_modules') - }) - } - } catch (e) { - console.log(`Cannot find ${packageManager}, skipping test`) - } -} + return util.packageAndEnsureResourcesPath(t, opts) + .then(resourcesPath => checkDependency(t, resourcesPath, 'electron', false)) +}) -util.testSinglePlatform('prune=false test', createPruneOptionTest, false, - 'npm devDependency should exist under app/node_modules') +util.testSinglePlatform('prune: false test', createPruneOptionTest, false, + 'package.json devDependency should exist under app/node_modules') diff --git a/usage.txt b/usage.txt index 3191822c..e1c4af16 100644 --- a/usage.txt +++ b/usage.txt @@ -52,9 +52,6 @@ no-prune do not prune devDependencies from the packaged app out the dir to put the app into at the end. Defaults to current working dir overwrite if output directory for a platform already exists, replaces it rather than skipping it -package-manager the package manager to use when pruning devDependencies. Supported package - managers: npm (default), cnpm, yarn. Specify --no-package-manager to prune - without using a package manager platform all, or one or more of: darwin, linux, mas, win32 (comma-delimited if multiple). Defaults to the host platform quiet Do not print informational or warning messages