-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Changes from 11 commits
866e2b7
60e160d
1cacb59
bcbb07d
0e80fe3
184e6f9
1d30d0c
8b98fee
9f27b5d
2bd5410
9308335
8354b61
b3ba099
b076b7d
8a3dc51
9680030
6f4a194
949c1df
b3ba28e
63c34f5
b26cb28
11a5c35
9c91de6
9e06479
bdb6485
2356c2a
4cf5d2d
06eecdd
40efbb3
89a668d
674d458
2030211
eb9c263
b32e731
37c412c
3343f28
6e07e6d
c462a4e
d2716cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
||
|
@@ -96,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 | ||
}; | ||
} | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding tests for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done :) |
||
if (!isLocalFile && this.options.autoinstall) { | ||
logger.status(emoji.progress, `Installing ${dep.name}...`); | ||
await installPackage( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be time for a config object to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
Path.dirname(asset.name), | ||
[dep.name], | ||
false, | ||
false, | ||
this.options.packageManager | ||
); | ||
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 (dep.name.startsWith('.')) { | ||
if (isLocalFile) { | ||
const absPath = Path.resolve(Path.dirname(asset.name), dep.name); | ||
err.message += ` at '${absPath}'`; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (I mean I suppose they could just I can remove it if you think we should, but I just don’t see any downsides to leaving it. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"', | ||
|
@@ -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 <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"', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,22 +3,67 @@ const config = require('./config'); | |
const path = require('path'); | ||
const promisify = require('./promisify'); | ||
const resolve = promisify(require('resolve')); | ||
const commandExists = require('command-exists').sync; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Pretty much, 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're already inside of a promise chain ( hasYarnInstalled = async () => {
try {
return await commandExists('yarn');
} catch (err) {
return false;
}
}
if (await hasYarnInstalled() && fs.exists(...)) {
...
} |
||
const logger = require('../Logger'); | ||
const fs = require('./fs'); | ||
|
||
async function install(dir, modules, installPeers = true) { | ||
let location = await config.resolve(dir, ['yarn.lock', 'package.json']); | ||
async function install( | ||
dir, | ||
modules, | ||
installPeers = true, | ||
saveDev = true, | ||
packageManager | ||
) { | ||
let projectRootLocation = dir; | ||
|
||
return new Promise((resolve, reject) => { | ||
let configFileLocation = await config.resolve(dir, [ | ||
'yarn.lock', | ||
'package.json' | ||
]); | ||
|
||
if (configFileLocation) | ||
projectRootLocation = path.dirname(configFileLocation); | ||
|
||
return new Promise(async (resolve, reject) => { | ||
let install; | ||
let options = { | ||
cwd: location ? path.dirname(location) : dir | ||
cwd: projectRootLocation | ||
}; | ||
|
||
if (location && path.basename(location) === 'yarn.lock') { | ||
install = spawn('yarn', ['add', ...modules, '--dev'], options); | ||
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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There’s a reason I separated them. I only want the Pretty much only if the user has yarn, but no There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I see your point though... if you want, I can remove it |
||
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." | ||
); | ||
} | ||
} | ||
} | ||
|
||
let commandToUse; | ||
if (packageManagerToUse === 'npm') { | ||
commandToUse = 'install'; | ||
} else { | ||
install = spawn('npm', ['install', ...modules, '--save-dev'], options); | ||
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); | ||
|
||
|
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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