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

Conversation

davidnagli
Copy link
Contributor

@davidnagli davidnagli commented Feb 14, 2018

Closes #376

  • Core functionality (should install any missing dependencies through npm)
  • Add CLI flag to disable it
  • Limit to only run in dev mode

Basically if Parcel runs into a missing dependency that's not a local file, it now attempts to npm install it using npm-programmatic
@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #805 into master will decrease coverage by 0.56%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
- Coverage   89.62%   89.06%   -0.57%     
==========================================
  Files          68       68              
  Lines        3442     3549     +107     
==========================================
+ Hits         3085     3161      +76     
- Misses        357      388      +31
Impacted Files Coverage Δ
src/utils/localRequire.js 93.33% <100%> (ø) ⬆️
src/Bundler.js 89.93% <22.72%> (-2.93%) ⬇️
src/utils/installPackage.js 69.81% <70.14%> (+7.9%) ⬆️
src/utils/pipeSpawn.js 68.18% <71.42%> (-22.73%) ⬇️
src/utils/PromiseQueue.js 69.73% <75%> (-25.62%) ⬇️
src/Logger.js 95.69% <85.71%> (-0.86%) ⬇️
src/assets/CSSAsset.js 81.73% <0%> (-4.35%) ⬇️
src/assets/TypeScriptAsset.js 97.14% <0%> (-2.86%) ⬇️
src/visitors/fs.js 79.86% <0%> (+0.69%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7b72f2...d2716cd. Read the comment docs.

@davidnagli davidnagli changed the title [WIP]] Automatically install missing dependencies, part 2 [WIP] Automatically install missing dependencies, part 2 Feb 14, 2018
src/Bundler.js Outdated
try {
try {
logger.status(emoji.progress, `Installing ${dep.name}...`);
await npm.install([dep.name], {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you can't use the installPackage helper that we already have? That also supports yarn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn’t know it existed 😬 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only problem is that it saves dependencies as a dev dependencies by default with no way to override that, so I’m gonna need to change it and refactor the code that uses it.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. You could just make dev dependency the default, and have an option to override it. Then you wouldn't need to change the existing calls.

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.
}

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.

@@ -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(...)) {
   ...
}

}

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.

+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.

src/Bundler.js Outdated

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.

src/Bundler.js Outdated
thrown = asset.generateErrorMessage(thrown);
let isLocalFile = dep.name.startsWith('.');

if (!isLocalFile) {
Copy link
Contributor

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

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 :)

src/Bundler.js Outdated

if (!isLocalFile) {
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?

src/Bundler.js Outdated
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 .

@brandon93s
Copy link
Contributor

Great start! 👍

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
@devongovett
Copy link
Member

IMO we should only do this in development mode (e.g. not parcel build). In CI environments, we definitely shouldn't be auto installing things for example.

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 :)
--no-autoinstall
--m, --package-manager
@davidnagli
Copy link
Contributor Author

Ready for review 😃

@davidnagli davidnagli changed the title [WIP] Automatically install missing dependencies, part 2 Automatically install missing dependencies, part 2 Feb 17, 2018
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'.
davidnagli and others added 3 commits February 20, 2018 20:04
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.
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/cli.js Outdated
@@ -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.

src/Bundler.js Outdated
@@ -332,14 +335,28 @@ class Bundler extends EventEmitter {
let thrown = err;

if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) {
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

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

// Default to npm
packageManagerToUse = 'npm';
// If the yarn command exists and we find a yarn.lock, use yarn
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.

if (commandExists('yarn') && await fs.exists(...))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There’s a reason I separated them. I only want the else to apply if await fs.exists(...) is false AND commandExists(‘yarn’) is true.

Pretty much only if the user has yarn, but no yarn.lock, then I want to trigger the warning. If they don’t have yarn, then no need to warn them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. The warning may be unnecessary. If they haven't explicitly asked for yarn, and there is no yarn.lock, then using npm without a warning seems valid - expected even.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty much the same edge case that I mentioned before applies. If you’re starting a new project, you’d want to know that you need to create a yarn.lock / --yarn and that Parcel isn’t using yarn.

I see your point though... if you want, I can remove it

src/Bundler.js Outdated
@@ -332,14 +335,28 @@ class Bundler extends EventEmitter {
let thrown = err;

if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) {
let isLocalFile = dep.name.startsWith('.');
// Attempt to install missing npm dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I’ve tried a few times but I’m gonna need to wait for #881 to land. Atm the second I add tests which use the test /input directory it causes a bunch of other flaky tests to go crazy.

Copy link
Contributor Author

@davidnagli davidnagli Mar 8, 2018

Choose a reason for hiding this comment

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

Done :)

src/Bundler.js Outdated
// Attempt to install missing npm dependencies
if (!isLocalFile && this.options.autoinstall) {
logger.status(emoji.progress, `Installing ${dep.name}...`);
await installPackage(
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be time for a config object to installPackage... 5 parameters!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

davidnagli and others added 9 commits March 6, 2018 12:33
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.
'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 :)
…von-autoinstall

# Conflicts:
#	src/Bundler.js
#	src/utils/installPackage.js
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.
* Renamed tests
* Changed afterEach cleanup to use async rimraf
This commit is 100% useless... I'm just trying to retrigger AppVeyor to make tests go green :)
@devongovett devongovett added this to the v1.7.0 milestone Mar 9, 2018
@davidnagli
Copy link
Contributor Author

Done...

@davidnagli davidnagli requested a review from a team March 13, 2018 01:03
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

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.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

I pushed a few fixes and improvements. Described them inline.

try {
return await this.resolveAsset(dep.name, asset.name);
} catch (err) {
let thrown = err;

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(
Copy link
Member

Choose a reason for hiding this comment

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

Added a check here to make sure we don't auto install dependencies of things inside node_modules, only application code. If a dependency is missing from within a node_modules folder, that is a bug in the package and so we should error.

throw thrown;
}
}

async installDep(asset, dep) {
let [moduleName] = this.resolver.getModuleParts(dep.name);
Copy link
Member

Choose a reason for hiding this comment

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

Used this function to get the module name to install. If you required e.g. lodash/cloneDeep in your app, we want to install lodash, not the entire dep name. The exception is scoped packages, e.g. @scope/module. In that case, we pass the whole thing. the getModuleParts function from the resolver handles both of these cases.

}

try {
await pipeSpawn(packageManager, args, {cwd});
Copy link
Member

Choose a reason for hiding this comment

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

Switched to using the pipeSpawn utility for this.


// npm doesn't auto-create a package.json when installing,
// so create an empty one if needed.
if (packageManager === 'npm' && !packageLocation) {
Copy link
Member

Choose a reason for hiding this comment

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

Added this to make sure we create a package.json when needed.


let hasYarn = null;
async function checkForYarnCommand() {
if (hasYarn != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Caching if we have yarn or not

let configName = configFile && path.basename(configFile);
if (!hasYarn || configName === 'package-lock.json') {
return 'npm';
}
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 hasYarn;
}

let queue = new PromiseQueue(install, {maxConcurrent: 1, retry: false});
Copy link
Member

Choose a reason for hiding this comment

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

Added a queue for install calls so that we can never have more than one install happening at the same time. Otherwise, npm will fail trying to write over the same files simultaneously.

}, opts));

cp.stdout.setEncoding('utf8').on('data', d => logger.writeRaw(d));
cp.stderr.setEncoding('utf8').on('data', d => logger.writeRaw(d));
Copy link
Member

Choose a reason for hiding this comment

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

pipeSpawn now uses the logger to write its output rather than directly piping to stdout. This creates a better experience when installing packages: when the install is complete, the output from yarn or npm is cleared since it is no longer relevant and the build continues. When an error occurs, it stays up to allow debugging.

@devongovett
Copy link
Member

Thanks for your work on this! Really excited about it. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants