refactor(installer): return more-helpful error message on fakeroot ENOENT #55
Conversation
src/installer.js
Outdated
| @@ -44,6 +44,14 @@ var spawn = function (options, command, args, callback) { | |||
| }) | |||
|
|
|||
| spawnedProcess.on('error', function (err) { | |||
| if (err.name = 'ENOENT') { | |||
| var installer = process.platform === 'darwin' ? 'brew' : 'apt-get' | |||
There was a problem hiding this comment.
Heh, this is not accurate on a non-Debian-derived Linux distro 😄
There was a problem hiding this comment.
I know 😢
@malept / @unindented any canonical sources you'd recommend for this, or would it be better to prepend an "e.g."?
There was a problem hiding this comment.
Better to prepend an "e.g.", waaaay too many Linux package managers out there.
src/installer.js
Outdated
| @@ -44,6 +44,14 @@ var spawn = function (options, command, args, callback) { | |||
| }) | |||
|
|
|||
| spawnedProcess.on('error', function (err) { | |||
| if (err.name = 'ENOENT') { | |||
There was a problem hiding this comment.
This should be err.name === 'ENOENT' right?
src/installer.js
Outdated
| var installer = process.platform === 'darwin' ? 'brew' : 'apt-get' | ||
|
|
||
| if (err.message.indexOf('fakeroot') > -1) { | ||
| err.message = 'Your system is missing the fakeroot package. Try ' + installer + ' install fakeroot' |
There was a problem hiding this comment.
Maybe put the command in quotes, like:
err.message = 'Your system is missing the fakeroot package. Try `' + installer + ' install fakeroot`'There was a problem hiding this comment.
👍 , had it like that originally but changed syntax to better match the file's style
|
Better error messages are always a good thing! |
|
Fantastic, thanks @unindented & @malept for the response + comments. @unindented—the error messages end up being pretty verbose given the construction of how the error is constructed for the callback below. I'm hoping this PR can address that as well, e.g. by providing a mechanism to handle special types of errors differently from others. Are there reasons you constructed the error as you did (e.g. not overwriting |
|
Regarding the error handling, I think I copied what |
src/installer.js
Outdated
| if (err.name === 'ENOENT') { | ||
| var installer = process.platform === 'darwin' ? 'brew' : 'apt-get' | ||
|
|
||
| if (err.message.indexOf('fakeroot') > -1) { |
There was a problem hiding this comment.
you could check err.syscall for spawn fakeroot or spawn dpkg, I do something similar in Electron Packager: https://github.com/electron-userland/electron-packager/blob/6f5f2934ca19a384991cc44b6c83e35322cbcd9d/win32.js#L29
Hi @unindented!
Was adding multi-platform installer support to electron-forge with @malept (i.e. "create a debian installer from a mac / darwin machine"), who recommended we provide more helpful error messages if the installer generation fails; this PR is a spike for that work.
This PR is kind of kludge-y, and isn't helpful when e.g. dpkg is not available on the host system. Ideally, a bit more work / thought would happen prior to merge, particularly around the downstream API for errors emitted to the provided callback.
Is this something you've been thinking about / had requests for? If not, is it something you're open to discussing?