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 2 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
55 changes: 36 additions & 19 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');

/**
* The Bundler is the main entry point. It resolves and loads assets,
Expand Down Expand Up @@ -328,26 +329,42 @@ 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('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the dependency be an absolute path, which would still be a local file? Trying to install in that scenario would blow up on npm install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya you’re right.

I would also have to add support for tilde paths etc now that #850 landed.

Copy link
Member

@devongovett devongovett Feb 23, 2018

Choose a reason for hiding this comment

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

You'll need something like let isLocalFile = /^[/~.]/.test(dep.name); in order to support the new resolver. See https://github.com/parcel-bundler/parcel/blob/master/src/Resolver.js#L88-L114


if (!isLocalFile) {
Copy link
Contributor

@brandon93s brandon93s Feb 15, 2018

Choose a reason for hiding this comment

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

Could invert the if to reduce some of the nesting here for readability.

Copy link
Contributor Author

@davidnagli davidnagli Feb 15, 2018

Choose a reason for hiding this comment

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

Not necessary once I used your try/catch split idea :)

try {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Nested try statements doesn't feel right. Can they be sequential?

// Install
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);
}

// Resolve
try {
  return await this.resolveAsset(dep.name, asset.name);
} catch (e) {
  if (dep.optional) {
    return;
  }

Or handle all in a single catch based on the error?

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

If install fails, should we continue or should it bail out? If the build would fail, could just bail out. If that's the case, could probably consolidate into a single try...catch .

}

return await this.resolveAsset(dep.name, asset.name);
} catch (e) {
if (dep.optional) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "optional" dependencies be installed automatically, or should we just skip these like before? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the looks of it, optional deps are pretty much dependencies that are supposed to get installed/bundled, but aren’t supposed to throw an error if they aren’t able to be resolved or installed.

So ideally we should attempt to install any optional deps, but we shouldn’t block the build if the install fails.

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;
}
Expand Down
17 changes: 12 additions & 5 deletions src/utils/installPackage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used in an async method. Should use the promise version exposed by command-exists with await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya that’s exactly what I originally wanted to do that 😃

But... because of the way the command-exists package is written, it’s doesn’t really work out. 😕

Pretty much, commandExists() conveys the result of whether or not it found the command by resolving the returned promise for “package found”, and rejecting the promise for “package not found”. This is a problem if we’re using async/await because await handles promise rejections by throwing. So pretty much whenever yarn isn’t found it’ll throw an error instead of nicely informing us.

I guess it’s possible to do it using a try/catch, but that’s really messy. The only other way to use the async version of command-exists is to just directly use the promise, but that’s really complicated since promise.finally hasn’t landed in Node yet and it would probably require nesting all the rest of the install code.

So I can still use the async version... just it won’t be as clean, and I don’t really think there’s that much of a performance loss from doing it synchronously, especially considering that the function it’s running in is gonna be executing asynchronously anyway.

Your call.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're already inside of a promise chain (return new Promise...) so you could use the Promise API with a bit of refactoring. Alternatively, you could create a hasYarnInstalled method:

hasYarnInstalled = async () => {
	try {
		return await commandExists('yarn');
	} catch (err) {
		return false;
	}
}

if (await hasYarnInstalled() && fs.exists(...)) {
   ...
}


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) => {
Expand All @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better preserve the old logic of location && path.basename(location) === 'yarn.lock'. Just because yarn is installed, doesn't mean it is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... is there a case where you wouldn’t want to use yarn if you already have it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure there is! I have yarn and I don't use it for every project. Specially when working with a team, everyone must use the same package manager for that project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it :)

We should keep both though, right? So we should check for the yarn lock file AND the yarn command, that way if they have a yarn lock but no yarn they can still use npm. Or is that also not necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see no need for keeping both checks, but I see no harm as well. Your call.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for keeping the yarn.lock check. I'm OK with both, but the lock check is going to be an indication of what the team / project is using.

command = 'yarn';
}

install = spawn(command, args, options);

install.stdout.pipe(process.stdout);
install.stderr.pipe(process.stderr);

Expand Down