From 866e2b731beb3bfa42840e2768f662a8e2bf57ec Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 13 Feb 2018 21:26:35 -0500 Subject: [PATCH 01/28] Automagically npm install missing dependencies Basically if Parcel runs into a missing dependency that's not a local file, it now attempts to npm install it using npm-programmatic --- package.json | 1 + src/Bundler.js | 56 +++++++++++++++++++++++++++++++++----------------- yarn.lock | 8 +++++++- 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/package.json b/package.json index c349f1770e2..94d1f0a7d0d 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "mkdirp": "^0.5.1", "node-forge": "^0.7.1", "node-libs-browser": "^2.0.0", + "npm-programmatic": "^0.0.10", "opn": "^5.1.0", "physical-cpu-count": "^2.0.0", "postcss": "^6.0.10", diff --git a/src/Bundler.js b/src/Bundler.js index 6c26f70e1ea..51cb8120a74 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -16,6 +16,7 @@ const config = require('./utils/config'); const emoji = require('./utils/emoji'); const loadEnv = require('./utils/env'); const PromiseQueue = require('./utils/PromiseQueue'); +const npm = require('npm-programmatic'); /** * The Bundler is the main entry point. It resolves and loads assets, @@ -328,26 +329,43 @@ class Bundler extends EventEmitter { let thrown = err; if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { - if (dep.optional) { - return; - } - - thrown.message = `Cannot resolve dependency '${dep.name}'`; - - // Add absolute path to the error message if the dependency specifies a relative path - if (dep.name.startsWith('.')) { - const absPath = Path.resolve(Path.dirname(asset.name), dep.name); - err.message += ` at '${absPath}'`; - } - - // Generate a code frame where the dependency was used - if (dep.loc) { - await asset.loadIfNeeded(); - thrown.loc = dep.loc; - thrown = asset.generateErrorMessage(thrown); + let isLocalFile = dep.name.startsWith('.'); + + if (!isLocalFile) { + try { + try { + logger.status(emoji.progress, `Installing ${dep.name}...`); + await npm.install([dep.name], { + cwd: Path.dirname(asset.name) + }); + } catch (npmError) { + logger.error(npmError.message); + } + + return await this.resolveAsset(dep.name, asset.name); + } catch (e) { + if (dep.optional) { + return; + } + + thrown.message = `Cannot resolve dependency '${dep.name}'`; + + // Add absolute path to the error message if the dependency specifies a relative path + if (isLocalFile) { + const absPath = Path.resolve(Path.dirname(asset.name), dep.name); + err.message += ` at '${absPath}'`; + } + + // Generate a code frame where the dependency was used + if (dep.loc) { + await asset.loadIfNeeded(); + thrown.loc = dep.loc; + thrown = asset.generateErrorMessage(thrown); + } + + thrown.fileName = asset.name; + } } - - thrown.fileName = asset.name; } throw thrown; } diff --git a/yarn.lock b/yarn.lock index d79a6dae3a7..4d05d995bb4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -825,7 +825,7 @@ block-stream@*: dependencies: inherits "~2.0.0" -bluebird@^3.0.5: +bluebird@^3.0.5, bluebird@^3.4.1: version "3.5.1" resolved "https://npm.cubos.io/bluebird/-/bluebird-3.5.1.tgz#d9551f9de98f1fcda1e683d17ee91a0602ee2eb9" @@ -4015,6 +4015,12 @@ npm-path@^2.0.2: dependencies: which "^1.2.10" +npm-programmatic@^0.0.10: + version "0.0.10" + resolved "https://registry.yarnpkg.com/npm-programmatic/-/npm-programmatic-0.0.10.tgz#9765820505d03083e57c6a7729bed515997a61c9" + dependencies: + bluebird "^3.4.1" + npm-run-path@^2.0.0: version "2.0.2" resolved "https://registry.yarnpkg.com/npm-run-path/-/npm-run-path-2.0.2.tgz#35a9232dfa35d7067b4cb2ddf2357b1871536c5f" From 60e160d2c2f6f5f74d9d5bfcdf566f4442049cbb Mon Sep 17 00:00:00 2001 From: David Nagli Date: Wed, 14 Feb 2018 15:53:21 -0500 Subject: [PATCH 02/28] Changed automatic install to use installPackage helper I had to add a parameter to installPackage to make saveDev optional, I also cleaned up the install code a bit in the proccess. I also fixed a bug where installPackage would naively use yarn as long as it found a "yarn.lock" which would cause issues if a user doesn't have yarn but is working on a project that commits its yarn.lock to the repo. So, I used the command-exists package to check first to prevent this issue from happening. --- package.json | 1 - src/Bundler.js | 7 +++---- src/utils/installPackage.js | 17 ++++++++++++----- yarn.lock | 8 +------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index 94d1f0a7d0d..c349f1770e2 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ "mkdirp": "^0.5.1", "node-forge": "^0.7.1", "node-libs-browser": "^2.0.0", - "npm-programmatic": "^0.0.10", "opn": "^5.1.0", "physical-cpu-count": "^2.0.0", "postcss": "^6.0.10", diff --git a/src/Bundler.js b/src/Bundler.js index 51cb8120a74..7e81d317d79 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -16,7 +16,7 @@ const config = require('./utils/config'); const emoji = require('./utils/emoji'); const loadEnv = require('./utils/env'); const PromiseQueue = require('./utils/PromiseQueue'); -const npm = require('npm-programmatic'); +const installPackage = require('./utils/installPackage'); /** * The Bundler is the main entry point. It resolves and loads assets, @@ -335,9 +335,8 @@ class Bundler extends EventEmitter { try { try { logger.status(emoji.progress, `Installing ${dep.name}...`); - await npm.install([dep.name], { - cwd: Path.dirname(asset.name) - }); + let dir = Path.dirname(asset.name); + await installPackage(dir, [dep.name], true, false); } catch (npmError) { logger.error(npmError.message); } diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index dd6774e896f..802f9fbc589 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -3,8 +3,9 @@ const config = require('./config'); const path = require('path'); const promisify = require('./promisify'); const resolve = promisify(require('resolve')); +const commandExists = require('command-exists').sync; -async function install(dir, modules, installPeers = true) { +async function install(dir, modules, installPeers = true, saveDev = true) { let location = await config.resolve(dir, ['yarn.lock', 'package.json']); return new Promise((resolve, reject) => { @@ -13,12 +14,18 @@ async function install(dir, modules, installPeers = true) { cwd: location ? path.dirname(location) : dir }; - if (location && path.basename(location) === 'yarn.lock') { - install = spawn('yarn', ['add', ...modules, '--dev'], options); - } else { - install = spawn('npm', ['install', ...modules, '--save-dev'], options); + let args = ['add', ...modules]; + if (saveDev) { + args.push('-D'); } + let command = 'npm'; + if (commandExists('yarn')) { + command = 'yarn'; + } + + install = spawn(command, args, options); + install.stdout.pipe(process.stdout); install.stderr.pipe(process.stderr); diff --git a/yarn.lock b/yarn.lock index 4d05d995bb4..d79a6dae3a7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -825,7 +825,7 @@ block-stream@*: dependencies: inherits "~2.0.0" -bluebird@^3.0.5, bluebird@^3.4.1: +bluebird@^3.0.5: version "3.5.1" resolved "https://npm.cubos.io/bluebird/-/bluebird-3.5.1.tgz#d9551f9de98f1fcda1e683d17ee91a0602ee2eb9" @@ -4015,12 +4015,6 @@ npm-path@^2.0.2: dependencies: which "^1.2.10" -npm-programmatic@^0.0.10: - version "0.0.10" - resolved "https://registry.yarnpkg.com/npm-programmatic/-/npm-programmatic-0.0.10.tgz#9765820505d03083e57c6a7729bed515997a61c9" - dependencies: - bluebird "^3.4.1" - npm-run-path@^2.0.0: version "2.0.2" resolved "https://registry.yarnpkg.com/npm-run-path/-/npm-run-path-2.0.2.tgz#35a9232dfa35d7067b4cb2ddf2357b1871536c5f" From bcbb07d1eb09d7644512abf70148ac109cd9a91e Mon Sep 17 00:00:00 2001 From: David Nagli Date: Thu, 15 Feb 2018 12:45:05 -0500 Subject: [PATCH 03/28] Added back yarn.lock check, improved yarn.lock & project root resolution I added back the check for yarn.lock but also kept the yarn command check. So now it checks for both the yarn command and a yarn.lock file. Also found bug where if you are using a flat project structure (ie entry point is not within a sub directory such as src), the 'location' variable would be null and result in yarn.lock never being found. So, I improved the logic to use the provided dir as a fallback to the config file directory, so if it comes back null dir get's used as the assumed project root and as a result stuff get's installed there and that's where we look for the yarn.lock --- src/utils/installPackage.js | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 802f9fbc589..5eeff8e821b 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -4,14 +4,21 @@ const path = require('path'); const promisify = require('./promisify'); const resolve = promisify(require('resolve')); const commandExists = require('command-exists').sync; +const logger = require('../Logger'); +const fs = require('./fs'); async function install(dir, modules, installPeers = true, saveDev = true) { - let location = await config.resolve(dir, ['yarn.lock', 'package.json']); + let configFileLocation = await config.resolve(dir, [ + 'yarn.lock', + 'package.json' + ]); - return new Promise((resolve, reject) => { + let projectRootLocation = path.dirname(configFileLocation) || dir; + + return new Promise(async (resolve, reject) => { let install; let options = { - cwd: location ? path.dirname(location) : dir + cwd: projectRootLocation }; let args = ['add', ...modules]; @@ -21,7 +28,14 @@ async function install(dir, modules, installPeers = true, saveDev = true) { let command = 'npm'; if (commandExists('yarn')) { - command = 'yarn'; + if (await fs.exists(path.join(projectRootLocation, 'yarn.lock'))) { + command = 'yarn'; + } else { + //todo: implement these flags + logger.warn( + "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --yarn or --npm flags to manually specify which package manager to use." + ); + } } install = spawn(command, args, options); From 184e6f929ba996379ce93c69465ad8ebe1667f84 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Fri, 16 Feb 2018 16:25:35 -0500 Subject: [PATCH 04/28] Bug fix I originally had it the other way but right before my last commit I decdided to clean up the code and refactor it this way. Problem is that while it looks cleaner, it throws an error when configFileLocation is null and get's passed into path.dirName as null. So ya... just moved it back to the original way :) --- src/utils/installPackage.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 5eeff8e821b..f8093be5963 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -8,12 +8,15 @@ const logger = require('../Logger'); const fs = require('./fs'); async function install(dir, modules, installPeers = true, saveDev = true) { + let projectRootLocation = dir; + let configFileLocation = await config.resolve(dir, [ 'yarn.lock', 'package.json' ]); - let projectRootLocation = path.dirname(configFileLocation) || dir; + if (configFileLocation) + projectRootLocation = path.dirname(configFileLocation); return new Promise(async (resolve, reject) => { let install; From 1d30d0cfffdc03e71798d0971686fd4445926c71 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Fri, 16 Feb 2018 16:41:20 -0500 Subject: [PATCH 05/28] refactor/cleanup code to fix the issues @brandon93s pointed out :) --- src/Bundler.js | 59 ++++++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index e7756d62780..57fc0966453 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -334,41 +334,34 @@ class Bundler extends EventEmitter { if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { let isLocalFile = dep.name.startsWith('.'); - + // Attempt to install missing npm dependencies if (!isLocalFile) { - try { - try { - logger.status(emoji.progress, `Installing ${dep.name}...`); - let dir = Path.dirname(asset.name); - await installPackage(dir, [dep.name], true, false); - } catch (npmError) { - logger.error(npmError.message); - } - - return await this.resolveAsset(dep.name, asset.name); - } catch (e) { - if (dep.optional) { - return; - } - - thrown.message = `Cannot resolve dependency '${dep.name}'`; - - // Add absolute path to the error message if the dependency specifies a relative path - if (isLocalFile) { - const absPath = Path.resolve(Path.dirname(asset.name), dep.name); - err.message += ` at '${absPath}'`; - } - - // Generate a code frame where the dependency was used - if (dep.loc) { - await asset.loadIfNeeded(); - thrown.loc = dep.loc; - thrown = asset.generateErrorMessage(thrown); - } - - thrown.fileName = asset.name; - } + logger.status(emoji.progress, `Installing ${dep.name}...`); + let dir = Path.dirname(asset.name); + await installPackage(dir, [dep.name], false, false); + return await this.resolveAsset(dep.name, asset.name); + } + + if (dep.optional) { + return; } + + thrown.message = `Cannot resolve dependency '${dep.name}'`; + + // Add absolute path to the error message if the dependency specifies a relative path + if (isLocalFile) { + const absPath = Path.resolve(Path.dirname(asset.name), dep.name); + err.message += ` at '${absPath}'`; + } + + // Generate a code frame where the dependency was used + if (dep.loc) { + await asset.loadIfNeeded(); + thrown.loc = dep.loc; + thrown = asset.generateErrorMessage(thrown); + } + + thrown.fileName = asset.name; } throw thrown; } From 8b98feed143c6fde42de0a36a7ccf1ef257978e5 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Fri, 16 Feb 2018 17:34:28 -0500 Subject: [PATCH 06/28] Dev mode check --- src/Bundler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Bundler.js b/src/Bundler.js index 57fc0966453..1dc53b95232 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -335,7 +335,7 @@ class Bundler extends EventEmitter { if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { let isLocalFile = dep.name.startsWith('.'); // Attempt to install missing npm dependencies - if (!isLocalFile) { + if (!isLocalFile && this.options.watch) { logger.status(emoji.progress, `Installing ${dep.name}...`); let dir = Path.dirname(asset.name); await installPackage(dir, [dep.name], false, false); From 9f27b5dabf48c728e657c97f720294dabbebe0cd Mon Sep 17 00:00:00 2001 From: David Nagli Date: Fri, 16 Feb 2018 21:33:33 -0500 Subject: [PATCH 07/28] Added autoinstall cli flags --no-autoinstall --m, --package-manager --- src/Bundler.js | 15 +++++++++++---- src/cli.js | 12 ++++++++++++ src/utils/installPackage.js | 35 ++++++++++++++++++++++++----------- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index 1dc53b95232..210ba205317 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -97,7 +97,9 @@ class Bundler extends EventEmitter { ? options.sourceMaps : !isProduction, hmrHostname: options.hmrHostname || '', - detailedReport: options.detailedReport || false + detailedReport: options.detailedReport || false, + autoinstall: (options.autoinstall || false) && !isProduction, + packageManager: options.packageManager }; } @@ -335,10 +337,15 @@ class Bundler extends EventEmitter { if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { let isLocalFile = dep.name.startsWith('.'); // Attempt to install missing npm dependencies - if (!isLocalFile && this.options.watch) { + if (!isLocalFile && this.options.autoinstall) { logger.status(emoji.progress, `Installing ${dep.name}...`); - let dir = Path.dirname(asset.name); - await installPackage(dir, [dep.name], false, false); + await installPackage( + Path.dirname(asset.name), + [dep.name], + false, + false, + this.options.packageManager + ); return await this.resolveAsset(dep.name, asset.name); } diff --git a/src/cli.js b/src/cli.js index 24eba68f1a2..d0438f1faf4 100755 --- a/src/cli.js +++ b/src/cli.js @@ -39,6 +39,12 @@ program .option('--no-hmr', 'disable hot module replacement') .option('--no-cache', 'disable the filesystem cache') .option('--no-source-maps', 'disable sourcemaps') + .option('--no-autoinstall', 'disable autoinstall') + .option( + '-m, --package-manager ', + 'set the package manger for autoinstall (npm or yarn)', + /^(npm|yarn)$/ + ) .option( '-t, --target [target]', 'set the runtime environment, either "node", "browser" or "electron". defaults to "browser"', @@ -65,6 +71,12 @@ program .option('--no-hmr', 'disable hot module replacement') .option('--no-cache', 'disable the filesystem cache') .option('--no-source-maps', 'disable sourcemaps') + .option('--no-autoinstall', 'disable autoinstall') + .option( + '-m, --package-manager ', + 'set the package manger for autoinstall (npm or yarn)', + /^(npm|yarn)$/ + ) .option( '-t, --target [target]', 'set the runtime environment, either "node", "browser" or "electron". defaults to "browser"', diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index f8093be5963..46199b82454 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -7,7 +7,13 @@ const commandExists = require('command-exists').sync; const logger = require('../Logger'); const fs = require('./fs'); -async function install(dir, modules, installPeers = true, saveDev = true) { +async function install( + dir, + modules, + installPeers = true, + saveDev = true, + packageManager +) { let projectRootLocation = dir; let configFileLocation = await config.resolve(dir, [ @@ -29,19 +35,26 @@ async function install(dir, modules, installPeers = true, saveDev = true) { args.push('-D'); } - let command = 'npm'; - if (commandExists('yarn')) { - if (await fs.exists(path.join(projectRootLocation, 'yarn.lock'))) { - command = 'yarn'; - } else { - //todo: implement these flags - logger.warn( - "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --yarn or --npm flags to manually specify which package manager to use." - ); + let packageManagerToUse; + if (packageManager) { + packageManagerToUse = packageManager; + } else { + // If no package manager specified, try to figure out which one to use: + // Default to npm + packageManagerToUse = 'npm'; + // If the yarn command exists and we find a yarn.lock, use yarn + if (commandExists('yarn')) { + if (await fs.exists(path.join(projectRootLocation, 'yarn.lock'))) { + packageManagerToUse = 'yarn'; + } else { + logger.warn( + "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --package-manager flag to explicitly specify which package manager to use." + ); + } } } - install = spawn(command, args, options); + install = spawn(packageManagerToUse, args, options); install.stdout.pipe(process.stdout); install.stderr.pipe(process.stderr); From 9308335c997dc373a4604c74cf97d1a5257db05b Mon Sep 17 00:00:00 2001 From: David Nagli Date: Sat, 17 Feb 2018 19:25:23 -0500 Subject: [PATCH 08/28] Fixed Node 6 Compatiblity Issues I relied on the fact that the latest version of npm also has the `npm add` command (just like yarn), so I was able to make it really cleanly use `add` as the command for both. Problem is that we also need to support legacy versions of node, and with it, legacy versions of npm (which don't support `npm add`). So I put it back to how it was originally pretty much... now it checks if it's npm or yarn, and based on that decides if it needs to use 'npm install' or 'yarn add'. --- src/utils/installPackage.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 46199b82454..31d0d09039d 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -30,11 +30,6 @@ async function install( cwd: projectRootLocation }; - let args = ['add', ...modules]; - if (saveDev) { - args.push('-D'); - } - let packageManagerToUse; if (packageManager) { packageManagerToUse = packageManager; @@ -54,6 +49,19 @@ async function install( } } + let commandToUse; + if (packageManagerToUse === 'npm') { + commandToUse = 'install'; + } else { + commandToUse = 'add'; + } + + let args = [commandToUse, ...modules]; + + if (saveDev) { + args.push('-D'); + } + install = spawn(packageManagerToUse, args, options); install.stdout.pipe(process.stdout); From 8354b6143c1f2683e5ffab193a9dc6c027b260b3 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 20 Feb 2018 20:03:31 -0500 Subject: [PATCH 09/28] Fixed refactoring mistake Caught a mistake caused by refactoring the basedir argument to dir, which caused the key in the config object to be incorrect and caused a bunch of issues. --- src/utils/installPackage.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 31d0d09039d..7f7d33e05ce 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -92,9 +92,7 @@ async function install( } async function installPeerDependencies(dir, name) { - let basedir = path.dirname(dir); - - const [resolved] = await resolve(name, {basedir}); + const [resolved] = await resolve(name, {basedir: dir}); const pkg = await config.load(resolved, ['package.json']); const peers = pkg.peerDependencies || {}; From b3ba099961c28304758d76914b6b3cd6586fcb3b Mon Sep 17 00:00:00 2001 From: David Nagli Date: Wed, 21 Feb 2018 08:24:45 -0500 Subject: [PATCH 10/28] Removed recursive config file search Before we were using config.resolve() to attempt to find where the nearest config file is, which is then used as the projectRootLocation. The problem is that config.resolve() also searches previous directories which causes oubvious problems since if you have a yarn.lock somewhere in a parent directory, it doesn't nessarely mean its from this project but config.resolve() would still find it. This was a big issue because it would break tests, since it would use Parcel's package.json / yarn.lock and end up installing packages in the Parcel repo root directory rather than inside the test input directory. Also, there's not really much use for a recursive search here, since all the code that uses install() provides the correct install directory, so we might as well just exect the correct input isntead of expecting the wrong input and then trying to hack together a way to figure out the right one. I suppose if we do find it nessary to do a reccursive resolve for project config files in the future then we can always just put it back to how it was and then statically compare the path strings to make sure we didn't escape the project directory. --- src/utils/installPackage.js | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 7f7d33e05ce..2c98201e111 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -7,24 +7,16 @@ const commandExists = require('command-exists').sync; const logger = require('../Logger'); const fs = require('./fs'); -async function install( +function install( dir, modules, installPeers = true, saveDev = true, packageManager ) { - let projectRootLocation = dir; - - let configFileLocation = await config.resolve(dir, [ - 'yarn.lock', - 'package.json' - ]); - - if (configFileLocation) - projectRootLocation = path.dirname(configFileLocation); - return new Promise(async (resolve, reject) => { + let projectRootLocation = dir; + let install; let options = { cwd: projectRootLocation From 8a3dc51f9b428ca35d213d37a2805d435af464e2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Fri, 23 Feb 2018 10:53:12 -0800 Subject: [PATCH 11/28] autoinstall improvements --- src/Bundler.js | 58 +++++++++++++++++++++++++------------ src/Logger.js | 5 ++++ src/utils/installPackage.js | 23 +++++++++++---- 3 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index b4216aa67f2..f10fabc9c2f 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -333,18 +333,14 @@ class Bundler extends EventEmitter { let thrown = err; if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { - let isLocalFile = dep.name.startsWith('.'); + let isLocalFile = /^[/~.]/.test(dep.name); + let fromNodeModules = asset.name.includes( + `${Path.sep}node_modules${Path.sep}` + ); + // Attempt to install missing npm dependencies - if (!isLocalFile && this.options.autoinstall) { - logger.status(emoji.progress, `Installing ${dep.name}...`); - await installPackage( - Path.dirname(asset.name), - [dep.name], - false, - false, - this.options.packageManager - ); - return await this.resolveAsset(dep.name, asset.name); + if (!isLocalFile && !fromNodeModules && this.options.autoinstall) { + return await this.installDep(asset, dep); } if (dep.optional) { @@ -359,19 +355,43 @@ class Bundler extends EventEmitter { err.message += ` at '${absPath}'`; } - // Generate a code frame where the dependency was used - if (dep.loc) { - await asset.loadIfNeeded(); - thrown.loc = dep.loc; - thrown = asset.generateErrorMessage(thrown); - } - - thrown.fileName = asset.name; + await this.throwDepError(asset, dep, thrown); } throw thrown; } } + async installDep(asset, dep) { + let moduleName = Path.normalize(dep.name).split(Path.sep)[0]; + logger.status(emoji.progress, `Installing ${moduleName}...`); + + try { + await installPackage( + asset.name, + [moduleName], + false, + false, + this.options.packageManager + ); + } catch (err) { + await this.throwDepError(asset, dep, err); + } + + return await this.resolveAsset(dep.name, asset.name); + } + + async throwDepError(asset, dep, err) { + // Generate a code frame where the dependency was used + if (dep.loc) { + await asset.loadIfNeeded(); + err.loc = dep.loc; + err = asset.generateErrorMessage(err); + } + + err.fileName = asset.name; + throw err; + } + async processAsset(asset, isRebuild) { if (isRebuild) { asset.invalidate(); diff --git a/src/Logger.js b/src/Logger.js index 00d08e8386d..b767d738b94 100644 --- a/src/Logger.js +++ b/src/Logger.js @@ -22,6 +22,11 @@ class Logger { this.chalk = new chalk.constructor({enabled: this.color}); } + writeRaw(message) { + this.lines += message.split('\n').length - 1; + process.stdout.write(message); + } + write(message, persistent = false) { if (!persistent) { this.lines += message.split('\n').length; diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 31d0d09039d..8c6d923478d 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -5,7 +5,6 @@ const promisify = require('./promisify'); const resolve = promisify(require('resolve')); const commandExists = require('command-exists').sync; const logger = require('../Logger'); -const fs = require('./fs'); async function install( dir, @@ -21,13 +20,20 @@ async function install( 'package.json' ]); - if (configFileLocation) + if (configFileLocation) { projectRootLocation = path.dirname(configFileLocation); + } return new Promise(async (resolve, reject) => { let install; let options = { - cwd: projectRootLocation + cwd: projectRootLocation, + env: Object.assign( + { + FORCE_COLOR: logger.color + }, + process.env + ) }; let packageManagerToUse; @@ -39,7 +45,10 @@ async function install( packageManagerToUse = 'npm'; // If the yarn command exists and we find a yarn.lock, use yarn if (commandExists('yarn')) { - if (await fs.exists(path.join(projectRootLocation, 'yarn.lock'))) { + if ( + !configFileLocation || + path.basename(configFileLocation) === 'yarn.lock' + ) { packageManagerToUse = 'yarn'; } else { logger.warn( @@ -64,8 +73,9 @@ async function install( install = spawn(packageManagerToUse, args, options); - install.stdout.pipe(process.stdout); - install.stderr.pipe(process.stderr); + // install.stdout.pipe(process.stdout); + install.stdout.setEncoding('utf8').on('data', d => logger.writeRaw(d)); + install.stderr.setEncoding('utf8').on('data', d => logger.writeRaw(d)); install.on('close', async code => { if (code !== 0) { @@ -86,6 +96,7 @@ async function install( ); } + logger.clear(); resolve(); }); }); From 949c1df4616310d90d55c9e119e3dffa98009a66 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 10:39:24 -0500 Subject: [PATCH 12/28] Removed redundent variable projectRootLocation After my previous commit, where I removed the reccursive config file search, this variable is useseless. --- src/utils/installPackage.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 2c98201e111..06336610bfd 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -15,11 +15,9 @@ function install( packageManager ) { return new Promise(async (resolve, reject) => { - let projectRootLocation = dir; - let install; let options = { - cwd: projectRootLocation + cwd: dir }; let packageManagerToUse; @@ -31,7 +29,7 @@ function install( packageManagerToUse = 'npm'; // If the yarn command exists and we find a yarn.lock, use yarn if (commandExists('yarn')) { - if (await fs.exists(path.join(projectRootLocation, 'yarn.lock'))) { + if (await fs.exists(path.join(dir, 'yarn.lock'))) { packageManagerToUse = 'yarn'; } else { logger.warn( From b3ba28ebc3ce17c32822e6bc4a24fa4ccf50a2ad Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 12:04:57 -0500 Subject: [PATCH 13/28] Added back reccursive yarn.lock resolution Ok lemme explain what happened (in case your wondering why I keep going back and forth with all this config.resolve stuff): Originally we would just attempt to resolve either package.json OR yarn.lock and then use that as the 'location' (later renamed to projectRootLocation). This caused a ton of issues, so I got rid of this conig.resolve entierely. Then I realized that yarn.lock won't get resolved if it's not directly in the directory, so now here I am adding back config.resolve(), but only for the purpuse of resolving the yarn lock file, and not for any of that project root stuff. --- src/utils/installPackage.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 06336610bfd..5ba9de7f857 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -1,11 +1,9 @@ const spawn = require('cross-spawn'); const config = require('./config'); -const path = require('path'); const promisify = require('./promisify'); const resolve = promisify(require('resolve')); const commandExists = require('command-exists').sync; const logger = require('../Logger'); -const fs = require('./fs'); function install( dir, @@ -20,6 +18,9 @@ function install( cwd: dir }; + // Attempt to resolve the yarn lockfile + let yarnLockFile = await config.resolve(dir, ['yarn.lock']); + let packageManagerToUse; if (packageManager) { packageManagerToUse = packageManager; @@ -29,7 +30,7 @@ function install( packageManagerToUse = 'npm'; // If the yarn command exists and we find a yarn.lock, use yarn if (commandExists('yarn')) { - if (await fs.exists(path.join(dir, 'yarn.lock'))) { + if (yarnLockFile) { packageManagerToUse = 'yarn'; } else { logger.warn( From 63c34f56201376e20139da219d20f5140678fa4a Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 12:18:22 -0500 Subject: [PATCH 14/28] CHANGED CONFIG.RESOLVE() TO CHECK THE DIRECORY BEFORE GOING TO PARENT Unit tests are passing, so I hope this doesn't break anything, but this fixes a huge subtle bug in the way config.resolve() works. Originally config.resolve() would set filepath to dirName(filePath) which would implicitly change the filepath to the parent directory, allowing the recursion to work. However, this is a problem when this statement is on the top, if the initial path passed in is a directory, because then that directory get's skipped. So I moved it to the bottom, after everything else runs, to make sure that we check the current directory before moving on to the parent (which makes sense) I'm not sure how this will behave if you pass in a path to a file rather than a directory. By the looks of it, it'll just preform a redundent recursive step (we should probably fix this too) --- src/utils/config.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils/config.js b/src/utils/config.js index ecb7e193290..cfa127ae63c 100644 --- a/src/utils/config.js +++ b/src/utils/config.js @@ -9,8 +9,6 @@ const PARSERS = { const existsCache = new Map(); async function resolve(filepath, filenames, root = path.parse(filepath).root) { - filepath = path.dirname(filepath); - // Don't traverse above the module root if (filepath === root || path.basename(filepath) === 'node_modules') { return null; @@ -27,7 +25,9 @@ async function resolve(filepath, filenames, root = path.parse(filepath).root) { } } - return resolve(filepath, filenames, root); + let prevDirFilepath = path.dirname(filepath); + + return resolve(prevDirFilepath, filenames, root); } async function load(filepath, filenames, root = path.parse(filepath).root) { From b26cb2867abf4bfe3aa1122564f33d2e4cfa2b62 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 12:21:47 -0500 Subject: [PATCH 15/28] Changed localrequire to pass in 'basedir' instead of 'path' to install() --- src/utils/localRequire.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/localRequire.js b/src/utils/localRequire.js index 100f52f23ce..7647df79963 100644 --- a/src/utils/localRequire.js +++ b/src/utils/localRequire.js @@ -13,7 +13,7 @@ async function localRequire(name, path, triedInstall = false) { resolved = resolve.sync(name, {basedir}); } catch (e) { if (e.code === 'MODULE_NOT_FOUND' && !triedInstall) { - await install(path, [name]); + await install(basedir, [name], true, true); return localRequire(name, path, true); } throw e; From 11a5c35d989fb599bb263aca6b417d3f4cc0be36 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 12:23:48 -0500 Subject: [PATCH 16/28] Added initial unit tests --- test/autoinstall.js | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 test/autoinstall.js diff --git a/test/autoinstall.js b/test/autoinstall.js new file mode 100644 index 00000000000..26e489e393d --- /dev/null +++ b/test/autoinstall.js @@ -0,0 +1,46 @@ +const assert = require('assert'); +const install = require('../src/utils/installPackage'); +const fs = require('fs'); +const rimraf = require('rimraf'); +const promisify = require('../src/utils/promisify'); +const primraf = promisify(rimraf); +const ncp = promisify(require('ncp')); +const inputDirPath = __dirname + '/input'; + +describe.only('autoinstall', function() { + before(async function() { + // Setup (clear the input dir and move integration test in) + await primraf(inputDirPath, {}); + await ncp(__dirname + '/integration/babel-default', inputDirPath); + }); + + describe('direct install', function() { + it('should install lodash using npm', async function() { + // Run install: + await install(inputDirPath, ['lodash'], false, true, 'npm'); + + /// Assert: + let pkg = require(inputDirPath + '/package.json'); + + assert(pkg.devDependencies['lodash']); + assert(fs.existsSync('node_modules/lodash')); + }); + + it('should install lodash using yarn', async function() { + // Run install: + await install(inputDirPath, ['lodash'], false, true, 'yarn'); + + /// Assert: + let pkg = require(inputDirPath + '/package.json'); + + assert(pkg.devDependencies['lodash']); + assert(fs.existsSync('node_modules/lodash')); + + assert(fs.existsSync('yarn.lock'), 'yarn.lock created'); + }); + }); + + after(function() { + rimraf.sync(inputDirPath); + }); +}); From 9c91de66315972f150bfa937bd7ad7ce002b588d Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 12:33:17 -0500 Subject: [PATCH 17/28] Whoops... removed test.only() filter from autoinstall unit tests --- test/autoinstall.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/autoinstall.js b/test/autoinstall.js index 26e489e393d..09c8139d8ae 100644 --- a/test/autoinstall.js +++ b/test/autoinstall.js @@ -7,7 +7,7 @@ const primraf = promisify(rimraf); const ncp = promisify(require('ncp')); const inputDirPath = __dirname + '/input'; -describe.only('autoinstall', function() { +describe('autoinstall', function() { before(async function() { // Setup (clear the input dir and move integration test in) await primraf(inputDirPath, {}); From bdb648501124f6ad2cc1f97cc9f5d7df7d9b2638 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 13:48:42 -0500 Subject: [PATCH 18/28] Added support for absolute and tilde paths --- src/Bundler.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Bundler.js b/src/Bundler.js index a7073f838dd..5109dc29c80 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -334,7 +334,8 @@ class Bundler extends EventEmitter { let thrown = err; if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { - let isLocalFile = dep.name.startsWith('.'); + // Check if dependency is a local file + let isLocalFile = /^[/~.]/.test(dep.name); // Attempt to install missing npm dependencies if (!isLocalFile && this.options.autoinstall) { logger.status(emoji.progress, `Installing ${dep.name}...`); From 2356c2ab534f1a16af0aa351c9a875cb64cdf862 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 14:23:56 -0500 Subject: [PATCH 19/28] Refactored parameters into config object --- src/utils/installPackage.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 5ba9de7f857..01e2cf7d4ee 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -5,14 +5,16 @@ const resolve = promisify(require('resolve')); const commandExists = require('command-exists').sync; const logger = require('../Logger'); -function install( - dir, - modules, - installPeers = true, - saveDev = true, - packageManager -) { +function install(config) { return new Promise(async (resolve, reject) => { + let { + dir, + modules, + installPeers = true, + saveDev = true, + packageManager + } = config; + let install; let options = { cwd: dir @@ -93,7 +95,7 @@ async function installPeerDependencies(dir, name) { } if (modules.length) { - await install(dir, modules, false); + await install({dir: dir, modules: modules, installPeers: false}); } } From 4cf5d2d72ee4471967edca48caf0ac85e24ad656 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 20:45:05 -0500 Subject: [PATCH 20/28] Fixed refactored usages, cleaned up unit tests Whoops turns out Webstorm doesn't automatically refactor usages when you use the automagical "convert parameters to config object" feature o_o I also cleaned up the unit tests a bit. --- src/Bundler.js | 14 ++++++------ src/utils/localRequire.js | 2 +- test/autoinstall.js | 48 ++++++++++++++++++++++++++++++--------- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index 5109dc29c80..817075dd5d7 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -339,13 +339,13 @@ class Bundler extends EventEmitter { // Attempt to install missing npm dependencies if (!isLocalFile && this.options.autoinstall) { logger.status(emoji.progress, `Installing ${dep.name}...`); - await installPackage( - Path.dirname(asset.name), - [dep.name], - false, - false, - this.options.packageManager - ); + await installPackage({ + dir: Path.dirname(asset.name), + modules: [dep.name], + installPeers: false, + saveDev: false, + packageManager: this.options.packageManager + }); return await this.resolveAsset(dep.name, asset.name); } diff --git a/src/utils/localRequire.js b/src/utils/localRequire.js index 7647df79963..c5e71b31c5c 100644 --- a/src/utils/localRequire.js +++ b/src/utils/localRequire.js @@ -13,7 +13,7 @@ async function localRequire(name, path, triedInstall = false) { resolved = resolve.sync(name, {basedir}); } catch (e) { if (e.code === 'MODULE_NOT_FOUND' && !triedInstall) { - await install(basedir, [name], true, true); + await install({dir: basedir, modules: [name]}); return localRequire(name, path, true); } throw e; diff --git a/test/autoinstall.js b/test/autoinstall.js index 09c8139d8ae..dba1e39a702 100644 --- a/test/autoinstall.js +++ b/test/autoinstall.js @@ -8,7 +8,7 @@ const ncp = promisify(require('ncp')); const inputDirPath = __dirname + '/input'; describe('autoinstall', function() { - before(async function() { + beforeEach(async function() { // Setup (clear the input dir and move integration test in) await primraf(inputDirPath, {}); await ncp(__dirname + '/integration/babel-default', inputDirPath); @@ -16,31 +16,57 @@ describe('autoinstall', function() { describe('direct install', function() { it('should install lodash using npm', async function() { + let pkgName = 'lodash'; + // Run install: - await install(inputDirPath, ['lodash'], false, true, 'npm'); + await install({ + dir: inputDirPath, + modules: [pkgName], + saveDev: true, + packageManager: 'npm' + }); /// Assert: - let pkg = require(inputDirPath + '/package.json'); + assert( + fs.existsSync( + inputDirPath + '/node_modules/' + pkgName, + 'lodash is installed after running install()' + ) + ); + + let pkg = fs.readFileSync(inputDirPath + '/package.json'); + pkg = JSON.parse(pkg); - assert(pkg.devDependencies['lodash']); - assert(fs.existsSync('node_modules/lodash')); + assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); }); it('should install lodash using yarn', async function() { + let pkgName = 'lodash'; + // Run install: - await install(inputDirPath, ['lodash'], false, true, 'yarn'); + await install({ + dir: inputDirPath, + modules: [pkgName], + saveDev: true, + packageManager: 'yarn' + }); /// Assert: - let pkg = require(inputDirPath + '/package.json'); + assert( + fs.existsSync( + inputDirPath + '/node_modules/' + pkgName, + 'lodash is installed after running install()' + ) + ); - assert(pkg.devDependencies['lodash']); - assert(fs.existsSync('node_modules/lodash')); + let pkg = fs.readFileSync(inputDirPath + '/package.json'); + pkg = JSON.parse(pkg); - assert(fs.existsSync('yarn.lock'), 'yarn.lock created'); + assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); }); }); - after(function() { + afterEach(function() { rimraf.sync(inputDirPath); }); }); From 06eecdd8b3dec43b1d0fc051adfd575a9bc10e4d Mon Sep 17 00:00:00 2001 From: David Nagli Date: Tue, 6 Mar 2018 20:49:39 -0500 Subject: [PATCH 21/28] Renamed 'config' to 'configObj' 'config' is already defined (as in 'config.resolve')... so by calling the parameter 'config' I was overiding that. Thanks JavaScript for not checking const overides in lexital scopes :) --- src/utils/installPackage.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index 01e2cf7d4ee..ca03120321d 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -5,7 +5,7 @@ const resolve = promisify(require('resolve')); const commandExists = require('command-exists').sync; const logger = require('../Logger'); -function install(config) { +function install(configObj) { return new Promise(async (resolve, reject) => { let { dir, @@ -13,7 +13,7 @@ function install(config) { installPeers = true, saveDev = true, packageManager - } = config; + } = configObj; let install; let options = { From 89a668d66429c39013467999ff14dbf16d9d0be9 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Wed, 7 Mar 2018 22:04:43 -0500 Subject: [PATCH 22/28] Cleanup / Refactoring - Split code into seperate functions I somehow ended up with these giant mega functions... so I went through and refactored everything into smaller functions. Should make everything much more readable. --- src/Bundler.js | 59 ++++++++------- src/utils/installPackage.js | 139 ++++++++++++++++-------------------- 2 files changed, 97 insertions(+), 101 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index 817075dd5d7..abf9cbf8a37 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -336,42 +336,51 @@ class Bundler extends EventEmitter { if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { // Check if dependency is a local file let isLocalFile = /^[/~.]/.test(dep.name); - // Attempt to install missing npm dependencies + + // If it's not a local file, attempt to install the dep if (!isLocalFile && this.options.autoinstall) { - logger.status(emoji.progress, `Installing ${dep.name}...`); - await installPackage({ - dir: Path.dirname(asset.name), - modules: [dep.name], - installPeers: false, - saveDev: false, - packageManager: this.options.packageManager - }); - return await this.resolveAsset(dep.name, asset.name); + let dir = Path.dirname(asset.name); + return await this.installDep(dep, dir); } + // If the dep is optional, return before we throw if (dep.optional) { return; } - thrown.message = `Cannot resolve dependency '${dep.name}'`; + thrown = await Bundler.generateDepError(asset, dep, thrown); + } + throw thrown; + } + } - // Add absolute path to the error message if the dependency specifies a relative path - if (isLocalFile) { - const absPath = Path.resolve(Path.dirname(asset.name), dep.name); - err.message += ` at '${absPath}'`; - } + async installDep(dep, dir) { + logger.status(emoji.progress, `Installing ${dep.name}...`); - // Generate a code frame where the dependency was used - if (dep.loc) { - await asset.loadIfNeeded(); - thrown.loc = dep.loc; - thrown = asset.generateErrorMessage(thrown); - } + await installPackage({ + dir, + modules: [dep.name], + installPeers: false, + saveDev: false, + packageManager: this.options.packageManager + }); - thrown.fileName = asset.name; - } - throw thrown; + return await this.resolveAsset(dep.name, dir); + } + + static async generateDepError(asset, dep, err) { + // Set the error message + err.message = `Cannot resolve dependency '${dep.name}'`; + + // Generate a code frame where the dependency was used + if (dep.loc) { + await asset.loadIfNeeded(); + err.loc = dep.loc; + err = asset.generateErrorMessage(err); } + err.fileName = asset.name; + + return err; } async processAsset(asset, isRebuild) { diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index ca03120321d..a0fc8983b43 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -2,86 +2,27 @@ const spawn = require('cross-spawn'); const config = require('./config'); const promisify = require('./promisify'); const resolve = promisify(require('resolve')); -const commandExists = require('command-exists').sync; +const commandExists = require('command-exists'); const logger = require('../Logger'); -function install(configObj) { - return new Promise(async (resolve, reject) => { - let { - dir, - modules, - installPeers = true, - saveDev = true, - packageManager - } = configObj; +async function install(configObj) { + let { + dir: cwd, + modules, + installPeers = true, + saveDev = true, + packageManager + } = configObj; - let install; - let options = { - cwd: dir - }; + packageManager = packageManager || (await determinePackageManager(cwd)); + let commandToUse = packageManager === 'npm' ? 'install' : 'add'; + let args = [commandToUse, ...modules, saveDev ? '-D' : null]; - // Attempt to resolve the yarn lockfile - let yarnLockFile = await config.resolve(dir, ['yarn.lock']); + await run(packageManager, args, {cwd}); - let packageManagerToUse; - if (packageManager) { - packageManagerToUse = packageManager; - } else { - // If no package manager specified, try to figure out which one to use: - // Default to npm - packageManagerToUse = 'npm'; - // If the yarn command exists and we find a yarn.lock, use yarn - if (commandExists('yarn')) { - if (yarnLockFile) { - packageManagerToUse = 'yarn'; - } else { - logger.warn( - "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --package-manager flag to explicitly specify which package manager to use." - ); - } - } - } - - let commandToUse; - if (packageManagerToUse === 'npm') { - commandToUse = 'install'; - } else { - commandToUse = 'add'; - } - - let args = [commandToUse, ...modules]; - - if (saveDev) { - args.push('-D'); - } - - install = spawn(packageManagerToUse, args, options); - - install.stdout.pipe(process.stdout); - install.stderr.pipe(process.stderr); - - install.on('close', async code => { - if (code !== 0) { - return reject(new Error(`Failed to install ${modules.join(', ')}.`)); - } - - if (!installPeers) { - return resolve(); - } - - try { - await Promise.all(modules.map(m => installPeerDependencies(dir, m))); - } catch (err) { - return reject( - new Error( - `Failed to install peerDependencies for ${modules.join(', ')}.` - ) - ); - } - - resolve(); - }); - }); + if (installPeers) { + await Promise.all(modules.map(m => installPeerDependencies(cwd, m))); + } } async function installPeerDependencies(dir, name) { @@ -95,8 +36,54 @@ async function installPeerDependencies(dir, name) { } if (modules.length) { - await install({dir: dir, modules: modules, installPeers: false}); + await install({dir, modules, installPeers: false}); + } +} + +async function determinePackageManager(cwd) { + let yarnLockFile = await config.resolve(cwd, ['yarn.lock']); + let yarnCommandExists = await checkForYarnCommand(); + + // If the yarn command exists and we find a yarn lockfile, use yarn + if (yarnCommandExists) { + if (yarnLockFile) { + return 'yarn'; + } else { + logger.warn( + "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --package-manager flag to explicitly specify which package manager to use." + ); + } } + + return 'npm'; +} + +async function checkForYarnCommand() { + try { + return await commandExists('yarn'); + } catch (err) { + return false; + } +} + +function run(...args) { + return new Promise((resolve, reject) => { + // Spawn the process + let childProcess = spawn(...args); + + // Setup outputs + childProcess.stdout.pipe(process.stdout); + childProcess.stderr.pipe(process.stderr); + + // Resolve the promise when the process finishes + childProcess.on('close', statusCode => { + if (statusCode === 0) { + resolve(); + } else { + reject(new Error(`Install failure: ${args}`)); + } + }); + }); } module.exports = install; From 674d458e2942e11850c078ddce80a268bd9cb8ab Mon Sep 17 00:00:00 2001 From: David Nagli Date: Wed, 7 Mar 2018 22:08:58 -0500 Subject: [PATCH 23/28] Cleaned up unit tests * Renamed tests * Changed afterEach cleanup to use async rimraf --- test/autoinstall.js | 74 ++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/test/autoinstall.js b/test/autoinstall.js index dba1e39a702..aa512c59399 100644 --- a/test/autoinstall.js +++ b/test/autoinstall.js @@ -14,59 +14,43 @@ describe('autoinstall', function() { await ncp(__dirname + '/integration/babel-default', inputDirPath); }); - describe('direct install', function() { - it('should install lodash using npm', async function() { - let pkgName = 'lodash'; + it('should install lodash using npm and save dev dependency to package.json', async function() { + let pkgName = 'lodash'; + + await install({ + dir: inputDirPath, + modules: [pkgName], + saveDev: true, + packageManager: 'npm' + }); - // Run install: - await install({ - dir: inputDirPath, - modules: [pkgName], - saveDev: true, - packageManager: 'npm' - }); + let expectedModulePath = inputDirPath + '/node_modules/' + pkgName; + assert(fs.existsSync(expectedModulePath), 'lodash is in node_modules'); - /// Assert: - assert( - fs.existsSync( - inputDirPath + '/node_modules/' + pkgName, - 'lodash is installed after running install()' - ) - ); + let pkg = fs.readFileSync(inputDirPath + '/package.json'); + pkg = JSON.parse(pkg); + assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); + }); - let pkg = fs.readFileSync(inputDirPath + '/package.json'); - pkg = JSON.parse(pkg); + it('should install lodash using yarn and save dev dependency to package.json', async function() { + let pkgName = 'lodash'; - assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); + await install({ + dir: inputDirPath, + modules: [pkgName], + saveDev: true, + packageManager: 'yarn' }); - it('should install lodash using yarn', async function() { - let pkgName = 'lodash'; - - // Run install: - await install({ - dir: inputDirPath, - modules: [pkgName], - saveDev: true, - packageManager: 'yarn' - }); + let expectedModulePath = inputDirPath + '/node_modules/' + pkgName; + assert(fs.existsSync(expectedModulePath), 'lodash is in node_modules'); - /// Assert: - assert( - fs.existsSync( - inputDirPath + '/node_modules/' + pkgName, - 'lodash is installed after running install()' - ) - ); - - let pkg = fs.readFileSync(inputDirPath + '/package.json'); - pkg = JSON.parse(pkg); - - assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); - }); + let pkg = fs.readFileSync(inputDirPath + '/package.json'); + pkg = JSON.parse(pkg); + assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep'); }); - afterEach(function() { - rimraf.sync(inputDirPath); + afterEach(async function() { + await primraf(inputDirPath); }); }); From 2030211129b4edb2419c5906a75bc049393b3a80 Mon Sep 17 00:00:00 2001 From: David Nagli Date: Thu, 8 Mar 2018 14:04:42 -0500 Subject: [PATCH 24/28] Retrigger CI This commit is 100% useless... I'm just trying to retrigger AppVeyor to make tests go green :) From 3343f28d98abf858b7c825f657cef0606dbaf7f9 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 17 Mar 2018 22:19:53 -0700 Subject: [PATCH 25/28] Autoinstall improvements --- src/Bundler.js | 58 ++++++++++------- src/Logger.js | 8 ++- src/utils/PromiseQueue.js | 24 +++++-- src/utils/config.js | 6 +- src/utils/installPackage.js | 121 ++++++++++++++++++++---------------- src/utils/localRequire.js | 2 +- src/utils/pipeSpawn.js | 16 ++++- 7 files changed, 146 insertions(+), 89 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index 0b331ec136d..420f7de8d2e 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -94,7 +94,8 @@ class Bundler extends EventEmitter { rootDir: Path.dirname(this.mainFile), sourceMaps: typeof options.sourceMaps === 'boolean' ? options.sourceMaps : true, - hmrHostname: options.hmrHostname || + hmrHostname: + options.hmrHostname || (options.target === 'electron' ? 'localhost' : ''), detailedReport: options.detailedReport || false, autoinstall: (options.autoinstall || false) && !isProduction, @@ -327,7 +328,7 @@ class Bundler extends EventEmitter { } } - async resolveDep(asset, dep) { + async resolveDep(asset, dep, install = true) { try { return await this.resolveAsset(dep.name, asset.name); } catch (err) { @@ -336,11 +337,18 @@ class Bundler extends EventEmitter { if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) { // Check if dependency is a local file let isLocalFile = /^[/~.]/.test(dep.name); + let fromNodeModules = asset.name.includes( + `${Path.sep}node_modules${Path.sep}` + ); // If it's not a local file, attempt to install the dep - if (!isLocalFile && this.options.autoinstall) { - let dir = Path.dirname(asset.name); - return await this.installDep(dep, dir); + if ( + !isLocalFile && + !fromNodeModules && + this.options.autoinstall && + install + ) { + return await this.installDep(asset, dep); } // If the dep is optional, return before we throw @@ -348,39 +356,43 @@ class Bundler extends EventEmitter { return; } - thrown = await Bundler.generateDepError(asset, dep, thrown); + thrown.message = `Cannot resolve dependency '${dep.name}'`; + if (isLocalFile) { + const absPath = Path.resolve(Path.dirname(asset.name), dep.name); + thrown.message += ` at '${absPath}'`; + } + + await this.throwDepError(asset, dep, thrown); } + throw thrown; } } - async installDep(dep, dir) { - logger.status(emoji.progress, `Installing ${dep.name}...`); - - await installPackage({ - dir, - modules: [dep.name], - installPeers: false, - saveDev: false, - packageManager: this.options.packageManager - }); + async installDep(asset, dep) { + let [moduleName] = this.resolver.getModuleParts(dep.name); + try { + await installPackage([moduleName], asset.name, { + saveDev: false, + packageManager: this.options.packageManager + }); + } catch (err) { + await this.throwDepError(asset, dep, err); + } - return await this.resolveAsset(dep.name, dir); + return await this.resolveDep(asset, dep, false); } - static async generateDepError(asset, dep, err) { - // Set the error message - err.message = `Cannot resolve dependency '${dep.name}'`; - + async throwDepError(asset, dep, err) { // Generate a code frame where the dependency was used if (dep.loc) { await asset.loadIfNeeded(); err.loc = dep.loc; err = asset.generateErrorMessage(err); } - err.fileName = asset.name; - return err; + err.fileName = asset.name; + throw err; } async processAsset(asset, isRebuild) { diff --git a/src/Logger.js b/src/Logger.js index b767d738b94..96eb8b5cef9 100644 --- a/src/Logger.js +++ b/src/Logger.js @@ -22,14 +22,18 @@ class Logger { this.chalk = new chalk.constructor({enabled: this.color}); } + countLines(message) { + return message.split('\n').reduce((p, line) => p + Math.ceil((line.length || 1) / process.stdout.columns), 0); + } + writeRaw(message) { - this.lines += message.split('\n').length - 1; + this.lines += this.countLines(message) - 1; process.stdout.write(message); } write(message, persistent = false) { if (!persistent) { - this.lines += message.split('\n').length; + this.lines += this.countLines(message); } this._log(message); diff --git a/src/utils/PromiseQueue.js b/src/utils/PromiseQueue.js index b02ac8008b1..d51366acc32 100644 --- a/src/utils/PromiseQueue.js +++ b/src/utils/PromiseQueue.js @@ -1,9 +1,12 @@ class PromiseQueue { - constructor(callback) { + constructor(callback, options = {}) { this.process = callback; + this.maxConcurrent = options.maxConcurrent || Infinity; + this.retry = options.retry !== false; this.queue = []; this.processing = new Set(); this.processed = new Set(); + this.numRunning = 0; this.runPromise = null; this.resolve = null; this.reject = null; @@ -14,7 +17,7 @@ class PromiseQueue { return; } - if (this.runPromise) { + if (this.runPromise && this.numRunning < this.maxConcurrent) { this._runJob(job, args); } else { this.queue.push([job, args]); @@ -41,13 +44,24 @@ class PromiseQueue { async _runJob(job, args) { try { + this.numRunning++; await this.process(job, ...args); this.processing.delete(job); this.processed.add(job); + this.numRunning--; this._next(); } catch (err) { - this.queue.push([job, args]); - this.reject(err); + this.numRunning--; + if (this.retry) { + this.queue.push([job, args]); + } else { + this.processing.delete(job); + } + + if (this.reject) { + this.reject(err); + } + this._reset(); } } @@ -58,7 +72,7 @@ class PromiseQueue { } if (this.queue.length > 0) { - while (this.queue.length > 0) { + while (this.queue.length > 0 && this.numRunning < this.maxConcurrent) { this._runJob(...this.queue.shift()); } } else if (this.processing.size === 0) { diff --git a/src/utils/config.js b/src/utils/config.js index cfa127ae63c..ecb7e193290 100644 --- a/src/utils/config.js +++ b/src/utils/config.js @@ -9,6 +9,8 @@ const PARSERS = { const existsCache = new Map(); async function resolve(filepath, filenames, root = path.parse(filepath).root) { + filepath = path.dirname(filepath); + // Don't traverse above the module root if (filepath === root || path.basename(filepath) === 'node_modules') { return null; @@ -25,9 +27,7 @@ async function resolve(filepath, filenames, root = path.parse(filepath).root) { } } - let prevDirFilepath = path.dirname(filepath); - - return resolve(prevDirFilepath, filenames, root); + return resolve(filepath, filenames, root); } async function load(filepath, filenames, root = path.parse(filepath).root) { diff --git a/src/utils/installPackage.js b/src/utils/installPackage.js index a0fc8983b43..296ccdb032e 100644 --- a/src/utils/installPackage.js +++ b/src/utils/installPackage.js @@ -1,32 +1,56 @@ -const spawn = require('cross-spawn'); const config = require('./config'); const promisify = require('./promisify'); const resolve = promisify(require('resolve')); const commandExists = require('command-exists'); const logger = require('../Logger'); +const emoji = require('./emoji'); +const pipeSpawn = require('./pipeSpawn'); +const PromiseQueue = require('./PromiseQueue'); +const path = require('path'); +const fs = require('./fs'); -async function install(configObj) { - let { - dir: cwd, - modules, - installPeers = true, - saveDev = true, - packageManager - } = configObj; +async function install(modules, filepath, options = {}) { + let {installPeers = true, saveDev = true, packageManager} = options; + + logger.status(emoji.progress, `Installing ${modules.join(', ')}...`); + + let packageLocation = await config.resolve(filepath, ['package.json']); + let cwd = packageLocation ? path.dirname(packageLocation) : process.cwd(); + + if (!packageManager) { + packageManager = await determinePackageManager(filepath); + } - packageManager = packageManager || (await determinePackageManager(cwd)); let commandToUse = packageManager === 'npm' ? 'install' : 'add'; - let args = [commandToUse, ...modules, saveDev ? '-D' : null]; + let args = [commandToUse, ...modules]; + if (saveDev) { + args.push('-D'); + } else if (packageManager === 'npm') { + args.push('--save'); + } + + // npm doesn't auto-create a package.json when installing, + // so create an empty one if needed. + if (packageManager === 'npm' && !packageLocation) { + await fs.writeFile(path.join(cwd, 'package.json'), '{}'); + } - await run(packageManager, args, {cwd}); + try { + await pipeSpawn(packageManager, args, {cwd}); + } catch (err) { + throw new Error(`Failed to install ${modules.join(', ')}.`); + } if (installPeers) { - await Promise.all(modules.map(m => installPeerDependencies(cwd, m))); + await Promise.all( + modules.map(m => installPeerDependencies(filepath, m, options)) + ); } } -async function installPeerDependencies(dir, name) { - const [resolved] = await resolve(name, {basedir: dir}); +async function installPeerDependencies(filepath, name, options) { + let basedir = path.dirname(filepath); + const [resolved] = await resolve(name, {basedir}); const pkg = await config.load(resolved, ['package.json']); const peers = pkg.peerDependencies || {}; @@ -36,54 +60,47 @@ async function installPeerDependencies(dir, name) { } if (modules.length) { - await install({dir, modules, installPeers: false}); + await install( + modules, + filepath, + Object.assign({}, options, {installPeers: false}) + ); } } -async function determinePackageManager(cwd) { - let yarnLockFile = await config.resolve(cwd, ['yarn.lock']); - let yarnCommandExists = await checkForYarnCommand(); - - // If the yarn command exists and we find a yarn lockfile, use yarn - if (yarnCommandExists) { - if (yarnLockFile) { - return 'yarn'; - } else { - logger.warn( - "Using NPM instead of Yarn. No 'yarn.lock' found in project directory, use the --package-manager flag to explicitly specify which package manager to use." - ); - } +async function determinePackageManager(filepath) { + let configFile = await config.resolve(filepath, [ + 'yarn.lock', + 'package-lock.json' + ]); + let hasYarn = await checkForYarnCommand(); + + // If Yarn isn't available, or there is a package-lock.json file, use npm. + let configName = configFile && path.basename(configFile); + if (!hasYarn || configName === 'package-lock.json') { + return 'npm'; } - return 'npm'; + return 'yarn'; } +let hasYarn = null; async function checkForYarnCommand() { + if (hasYarn != null) { + return hasYarn; + } + try { - return await commandExists('yarn'); + hasYarn = await commandExists('yarn'); } catch (err) { - return false; + hasYarn = false; } -} -function run(...args) { - return new Promise((resolve, reject) => { - // Spawn the process - let childProcess = spawn(...args); - - // Setup outputs - childProcess.stdout.pipe(process.stdout); - childProcess.stderr.pipe(process.stderr); - - // Resolve the promise when the process finishes - childProcess.on('close', statusCode => { - if (statusCode === 0) { - resolve(); - } else { - reject(new Error(`Install failure: ${args}`)); - } - }); - }); + return hasYarn; } -module.exports = install; +let queue = new PromiseQueue(install, {maxConcurrent: 1, retry: false}); +module.exports = function(...args) { + queue.add(...args); + return queue.run(); +}; diff --git a/src/utils/localRequire.js b/src/utils/localRequire.js index c5e71b31c5c..9744ed1a371 100644 --- a/src/utils/localRequire.js +++ b/src/utils/localRequire.js @@ -13,7 +13,7 @@ async function localRequire(name, path, triedInstall = false) { resolved = resolve.sync(name, {basedir}); } catch (e) { if (e.code === 'MODULE_NOT_FOUND' && !triedInstall) { - await install({dir: basedir, modules: [name]}); + await install([name], path); return localRequire(name, path, true); } throw e; diff --git a/src/utils/pipeSpawn.js b/src/utils/pipeSpawn.js index 3dad9ad4393..a89c1b9711b 100644 --- a/src/utils/pipeSpawn.js +++ b/src/utils/pipeSpawn.js @@ -1,9 +1,18 @@ const spawn = require('cross-spawn'); +const logger = require('../Logger'); function pipeSpawn(cmd, params, opts) { - const cp = spawn(cmd, params, opts); - cp.stdout.pipe(process.stdout); - cp.stderr.pipe(process.stderr); + const cp = spawn(cmd, params, Object.assign({ + env: Object.assign({ + FORCE_COLOR: logger.color, + npm_config_color: logger.color ? 'always': '', + npm_config_progress: true + }, process.env) + }, opts)); + + cp.stdout.setEncoding('utf8').on('data', d => logger.writeRaw(d)); + cp.stderr.setEncoding('utf8').on('data', d => logger.writeRaw(d)); + return new Promise((resolve, reject) => { cp.on('error', reject); cp.on('close', function(code) { @@ -11,6 +20,7 @@ function pipeSpawn(cmd, params, opts) { return reject(new Error(cmd + ' failed.')); } + logger.clear(); return resolve(); }); }); From 6e07e6d94978943be5986de34c3e77e156619683 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 17 Mar 2018 22:28:28 -0700 Subject: [PATCH 26/28] Fix tests --- test/autoinstall.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/autoinstall.js b/test/autoinstall.js index aa512c59399..240f74e8b2a 100644 --- a/test/autoinstall.js +++ b/test/autoinstall.js @@ -17,9 +17,7 @@ describe('autoinstall', function() { it('should install lodash using npm and save dev dependency to package.json', async function() { let pkgName = 'lodash'; - await install({ - dir: inputDirPath, - modules: [pkgName], + await install([pkgName], inputDirPath + '/test.js', { saveDev: true, packageManager: 'npm' }); @@ -35,9 +33,7 @@ describe('autoinstall', function() { it('should install lodash using yarn and save dev dependency to package.json', async function() { let pkgName = 'lodash'; - await install({ - dir: inputDirPath, - modules: [pkgName], + await install([pkgName], inputDirPath + '/test.js', { saveDev: true, packageManager: 'yarn' }); From c462a4ebfa3964fa4a0fa002c3b049ad06c470e1 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 17 Mar 2018 22:32:26 -0700 Subject: [PATCH 27/28] Remove explicit package-manager argument for now --- src/Bundler.js | 8 ++------ src/cli.js | 10 ---------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/src/Bundler.js b/src/Bundler.js index 420f7de8d2e..578851cba3c 100644 --- a/src/Bundler.js +++ b/src/Bundler.js @@ -98,8 +98,7 @@ class Bundler extends EventEmitter { options.hmrHostname || (options.target === 'electron' ? 'localhost' : ''), detailedReport: options.detailedReport || false, - autoinstall: (options.autoinstall || false) && !isProduction, - packageManager: options.packageManager + autoinstall: (options.autoinstall || false) && !isProduction }; } @@ -372,10 +371,7 @@ class Bundler extends EventEmitter { async installDep(asset, dep) { let [moduleName] = this.resolver.getModuleParts(dep.name); try { - await installPackage([moduleName], asset.name, { - saveDev: false, - packageManager: this.options.packageManager - }); + await installPackage([moduleName], asset.name, {saveDev: false}); } catch (err) { await this.throwDepError(asset, dep, err); } diff --git a/src/cli.js b/src/cli.js index dd29e85be76..a90f57895b2 100755 --- a/src/cli.js +++ b/src/cli.js @@ -40,11 +40,6 @@ program .option('--no-cache', 'disable the filesystem cache') .option('--no-source-maps', 'disable sourcemaps') .option('--no-autoinstall', 'disable autoinstall') - .option( - '-m, --package-manager ', - 'set the package manger for autoinstall (npm or yarn)', - /^(npm|yarn)$/ - ) .option( '-t, --target [target]', 'set the runtime environment, either "node", "browser" or "electron". defaults to "browser"', @@ -81,11 +76,6 @@ program .option('--no-cache', 'disable the filesystem cache') .option('--no-source-maps', 'disable sourcemaps') .option('--no-autoinstall', 'disable autoinstall') - .option( - '-m, --package-manager ', - 'set the package manger for autoinstall (npm or yarn)', - /^(npm|yarn)$/ - ) .option( '-t, --target [target]', 'set the runtime environment, either "node", "browser" or "electron". defaults to "browser"', From d2716cde963a282310d7b683deb759ca999cec56 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 17 Mar 2018 22:45:16 -0700 Subject: [PATCH 28/28] Fix line counting on windows --- src/Logger.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Logger.js b/src/Logger.js index 96eb8b5cef9..3e8aa5d65a6 100644 --- a/src/Logger.js +++ b/src/Logger.js @@ -23,7 +23,13 @@ class Logger { } countLines(message) { - return message.split('\n').reduce((p, line) => p + Math.ceil((line.length || 1) / process.stdout.columns), 0); + return message.split('\n').reduce((p, line) => { + if (process.stdout.columns) { + return p + Math.ceil((line.length || 1) / process.stdout.columns); + } + + return p + 1; + }, 0); } writeRaw(message) {