ENHANCEMENT: update-checker.js should use environment http_proxy if detected #3488
Conversation
| @@ -53,7 +53,8 @@ function cli(options) { | |||
| }; | |||
|
|
|||
| var defaultUpdateCheckerOptions = { | |||
| checkForUpdates: true | |||
| checkForUpdates: true, | |||
| httpProxy: process.env['http_proxy'] | |||
There was a problem hiding this comment.
our convention is all caps for env vars.
@rwjblue r?
There was a problem hiding this comment.
although the npm settings seem to be lower-case. Is it possible for us to read these settings via npm as a lib?
There was a problem hiding this comment.
Yes, agree. I'll check into that.
|
@xomaczar CI seems to not agree with this PR yet. |
| }); | ||
| }).then(function(version) { | ||
|
|
||
| }.bind(this)).then(function(version) { |
There was a problem hiding this comment.
this one? I am accessing this.settings.httpProxy, although it can be moved out where bind(this) will not be needed.
There was a problem hiding this comment.
@stefanpenner I ran npm test as described in docs before making this PR. All tests were passing. I'll look into that as well
There was a problem hiding this comment.
npm test is only the fast tests, npm run test-all includes the slow acceptance tests.
There was a problem hiding this comment.
updated docs: 5788bdb
|
@stefanpenner I need help, I ran npm run test-all, everything passed, except 2 pending. I squashed my changes, but PR does not reflect that. |
|
This is failing because the latest testem version (0.7.4) is missing a dependency. I submitted testem/testem#515 to fix this upstream. |
|
Testem 0.7.5 includes the fix, sorry about that. |
where? |
| })); | ||
|
|
||
| res.on('error', reject); | ||
| npm.load(npmViewConfig, function(err) { |
There was a problem hiding this comment.
we can use denodeify https://github.com/ember-cli/ember-cli/blob/master/lib/ext/promise.js#L13-L21
docs: https://github.com/tildeio/rsvp.js/blob/master/lib/rsvp/node.js#L73-L201
var load = Promise.denodeify(npm.load);
var view = Promise.denodeify(npm.commans.view);now stuff is all promisey and wonderful
There was a problem hiding this comment.
Promisey it is. Update to follow
|
@stefanpenner would u like for me to squash those 3 commits |
| return this.saveVersionInformation(version); | ||
| }.bind(this)) | ||
| .catch(function(error) { | ||
| this.ui.writeLine('There was an error checking NPM for an update: ' + error); |
There was a problem hiding this comment.
we have a this.ui.writeError, but the throw should handle that already, i suspect only the error.message is needed her.
There was a problem hiding this comment.
@stefanpenner you mean just this.ui.writeLine(e.message)? I haven't looked at the calling code, but according to your statement we're catching all rejections upstream, do we even need catch here?
There was a problem hiding this comment.
https://github.com/ember-cli/ember-cli/blob/master/lib/ui/index.js#L84-L93
which gets called for any error throw: https://github.com/ember-cli/ember-cli/blob/master/lib/cli/cli.js#L98
via: https://github.com/ember-cli/ember-cli/blob/master/lib/cli/cli.js#L136-L143
I believe, we just need error && typeof error === 'object' && error.message || '' no need in printing the error twice if it happens
@rwjblue r ?
There was a problem hiding this comment.
Why are we trying to catch those errors/rejections in https://github.com/xomaczar/ember-cli/blob/master/lib/models/update-checker.js#L86-L89.
The only way any of the those promises get rejected is if any errors are thrown inside then()s, we don't explicitly reject() anything in any then(...). Let's just https://github.com/ember-cli/ember-cli/blob/master/lib/cli/cli.js#L98 catch and log it.
i.e. I am suggesting to remove the catch() in update-checker.js and let the error bubble up.
There was a problem hiding this comment.
yes that sounds correct
|
@xomaczar not a big deal, i left one suggestion, then i think we are good to go. |
|
@stefanpenner There is one case where updatechecker will still throw exception, therefore stopping any command execution with |
|
@xomaczar so just log and message and more on if that is the error we see? Actually, maybe this should never re-throw and always just log and error but handle the rejection. It does seem silly that updateChecker could stop someone from developing. |
|
cc @twokul |
|
Agree just catch log, but don't re-throw, tne promise gets resolved, user sees the issue but able to continue...Hopefully last change |
|
@xomaczar thanks for working through all these changes with us :) Let me know when you think its good for another review, and yes lets hope to get it in asap :) |
|
@xomaczar @stefanpenner I assume this got resolved? |
|
@stefanpenner happy to be involved. Try to push it in tonight. |
👍 we are happy to have you involved, feel free to share feedback to help us all make contributing better. |
|
@stefanpenner ready for another review |
ENHANCEMENT: update-checker.js should use environment http_proxy if detected
|
👍 |
I am working behind corporate proxy and every command I provide to ember-cli (except test & update) result in an error without ability to proceed/recover.