Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically install missing dependencies, part 2 #805

Merged
merged 39 commits into from
Mar 18, 2018
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
866e2b7
Automagically npm install missing dependencies
davidnagli Feb 14, 2018
60e160d
Changed automatic install to use installPackage helper
davidnagli Feb 14, 2018
1cacb59
Merge branch 'master' into master
davidnagli Feb 14, 2018
bcbb07d
Added back yarn.lock check, improved yarn.lock & project root resolution
davidnagli Feb 15, 2018
0e80fe3
Merge remote-tracking branch 'origin/master'
davidnagli Feb 15, 2018
184e6f9
Bug fix
davidnagli Feb 16, 2018
1d30d0c
refactor/cleanup code to fix the issues @brandon93s pointed out
davidnagli Feb 16, 2018
8b98fee
Dev mode check
davidnagli Feb 16, 2018
9f27b5d
Added autoinstall cli flags
davidnagli Feb 17, 2018
2bd5410
Merge remote-tracking branch 'upstream/master'
davidnagli Feb 17, 2018
9308335
Fixed Node 6 Compatiblity Issues
davidnagli Feb 18, 2018
8354b61
Fixed refactoring mistake
davidnagli Feb 21, 2018
b3ba099
Removed recursive config file search
davidnagli Feb 21, 2018
b076b7d
Merge branch 'master' of https://github.com/davidnagli/parcel into da…
devongovett Feb 22, 2018
8a3dc51
autoinstall improvements
devongovett Feb 23, 2018
9680030
Merge branch 'master' of github.com:parcel-bundler/parcel into davidn…
devongovett Mar 6, 2018
6f4a194
Merge branch 'davidnagli-master' into devon-autoinstall
devongovett Mar 6, 2018
949c1df
Removed redundent variable projectRootLocation
davidnagli Mar 6, 2018
b3ba28e
Added back reccursive yarn.lock resolution
davidnagli Mar 6, 2018
63c34f5
CHANGED CONFIG.RESOLVE() TO CHECK THE DIRECORY BEFORE GOING TO PARENT
davidnagli Mar 6, 2018
b26cb28
Changed localrequire to pass in 'basedir' instead of 'path' to install()
davidnagli Mar 6, 2018
11a5c35
Added initial unit tests
davidnagli Mar 6, 2018
9c91de6
Whoops... removed test.only() filter from autoinstall unit tests
davidnagli Mar 6, 2018
9e06479
Merge branch 'master' of https://github.com/davidnagli/parcel
davidnagli Mar 6, 2018
bdb6485
Added support for absolute and tilde paths
davidnagli Mar 6, 2018
2356c2a
Refactored parameters into config object
davidnagli Mar 6, 2018
4cf5d2d
Fixed refactored usages, cleaned up unit tests
davidnagli Mar 7, 2018
06eecdd
Renamed 'config' to 'configObj'
davidnagli Mar 7, 2018
40efbb3
Merge branch 'master' of https://github.com/davidnagli/parcel into de…
devongovett Mar 7, 2018
89a668d
Cleanup / Refactoring - Split code into seperate functions
davidnagli Mar 8, 2018
674d458
Cleaned up unit tests
davidnagli Mar 8, 2018
2030211
Retrigger CI
davidnagli Mar 8, 2018
eb9c263
Merge branch 'master' into master
davidnagli Mar 13, 2018
b32e731
Merge branch 'master' of https://github.com/davidnagli/parcel into de…
devongovett Mar 17, 2018
37c412c
Merge branch 'master' of github.com:parcel-bundler/parcel into devon-…
devongovett Mar 18, 2018
3343f28
Autoinstall improvements
devongovett Mar 18, 2018
6e07e6d
Fix tests
devongovett Mar 18, 2018
c462a4e
Remove explicit package-manager argument for now
devongovett Mar 18, 2018
d2716cd
Fix line counting on windows
devongovett Mar 18, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 44 additions & 18 deletions src/Bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 installPackage = require('./utils/installPackage');
const bundleReport = require('./utils/bundleReport');
const prettifyTime = require('./utils/prettifyTime');

Expand Down Expand Up @@ -93,10 +94,11 @@ 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
detailedReport: options.detailedReport || false,
autoinstall: (options.autoinstall || false) && !isProduction,
packageManager: options.packageManager
};
}

Expand Down Expand Up @@ -332,31 +334,55 @@ 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}'`;
// Check if dependency is a local file
let isLocalFile = /^[/~.]/.test(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}'`;
// 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);
}

// Generate a code frame where the dependency was used
if (dep.loc) {
await asset.loadIfNeeded();
thrown.loc = dep.loc;
thrown = asset.generateErrorMessage(thrown);
// If the dep is optional, return before we throw
if (dep.optional) {
return;
}

thrown.fileName = asset.name;
thrown = await Bundler.generateDepError(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
});

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) {
if (isRebuild) {
asset.invalidate();
Expand Down
12 changes: 12 additions & 0 deletions src/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <packageManager>',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this option really necessary? Our approach to inferring the package manager seems pretty solid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s true, but I don’t really see any downsides to giving the users manual control if necessary.

One scenario that I would imagine will be particularly problematic is if a user is starting a brand new project with Parcel and has no dependencies yet. They try to add their first import statement, and Parcel automatically uses npm because it doesn’t see a yarn.lock, even though the user would rather use yarn. In that case they would want to use the -m yarn flag to force it to use yarn.

(I mean I suppose they could just touch yarn.lock to generate an empty one, but that’s kinda hacky and just another point for confusion)

I can remove it if you think we should, but I just don’t see any downsides to leaving it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to remove it for now. I think our approach is pretty solid. If someone wants to open a bug with a good reason to add an option for it, they can.

'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"',
Expand Down Expand Up @@ -74,6 +80,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 <packageManager>',
'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"',
Expand Down
6 changes: 3 additions & 3 deletions src/utils/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
107 changes: 65 additions & 42 deletions src/utils/installPackage.js
Original file line number Diff line number Diff line change
@@ -1,55 +1,32 @@
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');
const logger = require('../Logger');

async function install(dir, modules, installPeers = true) {
let location = await config.resolve(dir, ['yarn.lock', 'package.json']);
async function install(configObj) {
let {
dir: cwd,
modules,
installPeers = true,
saveDev = true,
packageManager
} = configObj;

return new Promise((resolve, reject) => {
let install;
let options = {
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);
}

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();
}
packageManager = packageManager || (await determinePackageManager(cwd));
let commandToUse = packageManager === 'npm' ? 'install' : 'add';
let args = [commandToUse, ...modules, saveDev ? '-D' : null];

try {
await Promise.all(modules.map(m => installPeerDependencies(dir, m)));
} catch (err) {
return reject(
new Error(
`Failed to install peerDependencies for ${modules.join(', ')}.`
)
);
}
await run(packageManager, args, {cwd});

resolve();
});
});
if (installPeers) {
await Promise.all(modules.map(m => installPeerDependencies(cwd, m)));
}
}

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 || {};

Expand All @@ -59,8 +36,54 @@ async function installPeerDependencies(dir, name) {
}

if (modules.length) {
await install(dir, modules, 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."
);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this logic slightly. Now we default to yarn if it is installed, but use npm if a package-lock.json file is found and yarn.lock is not, or yarn is not installed.


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;
2 changes: 1 addition & 1 deletion src/utils/localRequire.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({dir: basedir, modules: [name]});
return localRequire(name, path, true);
}
throw e;
Expand Down
56 changes: 56 additions & 0 deletions test/autoinstall.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
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('autoinstall', function() {
beforeEach(async function() {
// Setup (clear the input dir and move integration test in)
await primraf(inputDirPath, {});
await ncp(__dirname + '/integration/babel-default', inputDirPath);
});

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'
});

let expectedModulePath = inputDirPath + '/node_modules/' + pkgName;
assert(fs.existsSync(expectedModulePath), 'lodash is in node_modules');

let pkg = fs.readFileSync(inputDirPath + '/package.json');
pkg = JSON.parse(pkg);
assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep');
});

it('should install lodash using yarn and save dev dependency to package.json', async function() {
Copy link
Member

@DeMoorJasper DeMoorJasper Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also add a test that tests the autoinstall from require.
For example just have a script like this that gets bundled and check if it's installed after test ran. (Or passed without errors and outputs the correct output)

const _ = require('lodash');
// Do something with lodash to check later...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be good to do in a followup PR. Maybe we can cover a few more of the different cases as well.

let pkgName = 'lodash';

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');

let pkg = fs.readFileSync(inputDirPath + '/package.json');
pkg = JSON.parse(pkg);
assert(pkg.devDependencies[pkgName], 'lodash is saved as a dev dep');
});

afterEach(async function() {
await primraf(inputDirPath);
});
});