Stop crashing when update-notifier fails #715
Conversation
| try { | ||
| updateNotifier({ pkg }).notify(); | ||
| } catch (err) { | ||
| logs.warn('Failed to check for newer version of the CLI'); |
There was a problem hiding this comment.
I might even be in favor of silently failing with this one because it seems like people who do have the problem will hit this virtually every time they use the CLI.
There was a problem hiding this comment.
in favor of silently failing with this one
Seconded
There was a problem hiding this comment.
Sounds good!
What we could to would be to only show the error the first time, if only there was some kind of library that made it very easy to store if we have previously shown the error or not... Oh, I know! Nah, just kidding ;)
| try { | ||
| updateNotifier({ pkg }).notify(); | ||
| } catch (err) { | ||
| CrashReporter.submit(err.stack); |
There was a problem hiding this comment.
Isn't this going to have the same net outcome?
There was a problem hiding this comment.
Hmm, do you mean that it will print a warning? Yes that is right, I'll take a look...
There was a problem hiding this comment.
It shouldn't because the error is caught. CrashReporter.submit doesn't exit the process.
There was a problem hiding this comment.
It prints INFO Crash Reported: http://crash-reporter.tessel.io/crashes?fingerprint=-0x173999f5 which is about as annoying as WARN Failed to check for newer versions of the CLI
| try { | ||
| updateNotifier({ pkg }).notify(); | ||
| } catch (err) { | ||
| CrashReporter.submit(err.stack, { silent: true }); |
There was a problem hiding this comment.
So, now it's quiet for the user, but http://crash-reporter.tessel.io/ is going to continue to amass these reports. I think the real fix is to simply suppress this error :| (or maybe the authors of configstore could just make the damn thing work without barfing all over the place)
There was a problem hiding this comment.
I thought it was a good idea to keep collecting the crash data, that way we can see how many users are impacted by this, and also help out with additional information that might be required for yeoman/update-notifier#76...
There was a problem hiding this comment.
Ok, I can agree to that then.
|
@rwaldron I need to go to bed now but feel free to make modifications to this branch and land it 👍 |
Good night! |
|
Tests added: 43db821 |
related: #714
ping @rwaldron @johnnyman727
This is a quick fix that will stop the t2 cli from crashing for the affected users. It will print a warning and then report the error to our crash server.
Another funny thing I noticed is that the crash reporter won't be able to upload crash data if we call
process.exit()to early, like we do when running onlyt2for example, but that's another issue altogether...