-
-
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 1 commit
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 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) { | ||
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. Could invert the 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. Not necessary once I used your try/catch split idea :) |
||
try { | ||
try { | ||
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. Hmm. Nested // 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}...`); | ||
await npm.install([dep.name], { | ||
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. Any reason you can't use the 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. Didn’t know it existed 😬 😂 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. 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. 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. 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. |
||
cwd: Path.dirname(asset.name) | ||
}); | ||
} catch (npmError) { | ||
logger.error(npmError.message); | ||
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. 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 |
||
} | ||
|
||
return await this.resolveAsset(dep.name, asset.name); | ||
} catch (e) { | ||
if (dep.optional) { | ||
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. Should "optional" dependencies be installed automatically, or should we just skip these like before? Thoughts? 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. 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; | ||
} | ||
|
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