-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(local-install): Add npmEnv
option to LocalInstaller
#7
Conversation
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.
Thanks for your PR! Looks good so far. I have a few remarks.
I'm also missing a small update in the readme and some tests. Do you need some help with that?
Are you planning to use it from the programmatic API and not the CLI?
super(); | ||
this.sourcesByTarget = resolve(sourcesByTarget); | ||
if (options) { |
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 we use Object.assign here? options = Object.assign({}, options);
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.
sure, good point
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.
Object.assign
can handle an undefined
value. So you can even remove this entire if
:
constructor(sourcesByTarget: ListByPackage, options?: Options) {
super();
this.sourcesByTarget = resolve(sourcesByTarget);
this.options = Object.assign({}, options);
}
src/LocalInstaller.ts
Outdated
@@ -1,3 +1,4 @@ | |||
import * as child_process from 'child_process'; |
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.
the ExecOptions
are actually re-exported from mz/child_process
(using export * from "child_process";
), so you should be able to use import { exec, ExecOptions } from 'mz/child_process';
instead.
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.
Oww, good to know, much more cleaner that way.
You've got to love mutation testing. Build failed because new code paths do not have unit tests:
|
Thanks for looking into this 🙂
I guess it would be difficult to use it from the CLI. I mean passing an object from CLI would be hard. Maybe there should be an option to pass it when using the CLI interface programmatically but not sure if it's worth it. |
Would you have a moment to take a look? |
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.
Sorry it took so long. I somehow missed the update from git. Thanks for the reminder
super(); | ||
this.sourcesByTarget = resolve(sourcesByTarget); | ||
if (options) { |
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.
Object.assign
can handle an undefined
value. So you can even remove this entire if
:
constructor(sourcesByTarget: ListByPackage, options?: Options) {
super();
this.sourcesByTarget = resolve(sourcesByTarget);
this.options = Object.assign({}, options);
}
It's released in v0.5.0: https://github.com/nicojs/node-install-local/blob/master/CHANGELOG.md |
Thanks for your time and for merging it in! 👍 |
Hello,
I have a plan to use your package in
meteor-desktop
but I've faced a problem when installing local packages that have native node modules. The compilation goes against current node and what I need is to compile them against the electron version. For that I need to pass additional env vars to npm. I have made this small change to be able to do that.I am a total newbie in typescript so I hope its typescriptish enough 🙂
Let me know if that can go into your next release.