Add error.signalDescription
#378
Conversation
|
|
|
|
index.d.ts
Outdated
| */ | ||
| signal?: string; | ||
|
|
||
| /** | ||
| A human-friendly description of the signal that was used to terminate the process. |
There was a problem hiding this comment.
Can you mention that it's already included in the default Execa error message? So users don't try to be smart and include it themselves and getting it double.
Can you also include a short example of what the description looks like for a signal? (readme too)
There was a problem hiding this comment.
We should also be explicit about that there might be signals with a signalDescription. So users should assume that signalDescription will be defined if signal is.
test/error.js
Outdated
| @@ -133,6 +133,15 @@ if (process.platform !== 'win32') { | |||
| t.is(signal, 'SIGINT'); | |||
| }); | |||
|
|
|||
| test('error.signalDescription is defined', async t => { | |||
| const cp = execa('noop'); | |||
There was a problem hiding this comment.
Let's try to use subprocess instead of cp everywhere. Maybe we should add it to https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/prevent-abbreviations.md
index.d.ts
Outdated
| @@ -254,9 +254,14 @@ declare namespace execa { | |||
| killed: boolean; | |||
|
|
|||
| /** | |||
| The signal that was used to terminate the process. | |||
| The name of the signal that was used to terminate the process. | |||
There was a problem hiding this comment.
Unrelated to this PR, but I just noticed, when something can be undefined, we should try to describe in what scenarios it's defined vs undefined. This can be a big help for the user in knowing when it will be used.
That's a tough one as I'm using Babel to transpile the library. I've tried to find an option or plugin to inline Babel helpers instead of having Babel inject |
|
Why are you even transpiling? Seems like you have a lot of tooling and output bloat/complexity just to be able to use import/export (as the target Node.js version supports most other things). All you really need is https://babeljs.io/docs/en/babel-plugin-transform-modules-commonjs |
|
Yes it's true the only thing I am using is ES modules there. The thing is I am using this setup with all my projects (which are of different sizes), so I can use other things like However on a small library like this, this does look rather silly. Actually I am looking into the best way to remove the |
|
Fixed. I removed the |
I also noticed that you're including source maps in the package. Seeing as you don't include the actual source in the package, the source map files are useless, so I would just not produce them. They are also not very valuable as Node.js doesn't use source maps. |
ehmicky
force-pushed
the
feature/signal-description
branch
from
7a4c458
to
cf3161a
Compare
The sources are included in the source map files in the
By default they are not used in Node.js for error stack traces. But this can be fixed with either:
It's also used by debuggers. For example if users are debugging some code using |
Oh, nice. I didn't know you could embed the source. |
At the moment we set
error.signalto eitherundefinedor the signal that terminated a process such as"SIGFPE". This PR addserror.signalDescriptionwhich is a human-friendly description of that signal such as"Floating point arithmetic error".This also enhances error messages accordingly such as:
instead of:
This uses
human-signals, a library I wrote which exports a simple plain object where the key is the signal name and the value the signal description. This works cross-platform.