Skip to content

feat(CordovaError): support for error cause & more#121

Merged
raphinesse merged 1 commit into
apache:masterfrom
raphinesse:error-cause
Mar 14, 2020
Merged

feat(CordovaError): support for error cause & more#121
raphinesse merged 1 commit into
apache:masterfrom
raphinesse:error-cause

Conversation

@raphinesse
Copy link
Copy Markdown
Contributor

@raphinesse raphinesse commented Dec 13, 2019

Motivation and Context

This allows us to properly wrap caught errors in a CordovaError:

try {
   builder.build(); // throws Error('foo not found')
} catch (err1) {
    throw new CordovaError('Build failed', err1));
}

Assume we catch above CordovaError as err2, then we can obtain some nicely formatted information from it:

console.log(err2.message);
CordovaError: Build failed: foo not found
console.log(CordovaError.fullStack(err2));
CordovaError: Build failed
    at ...
    at ...
caused by: foo not found
    at ...
    at ...

This is a follow-up to #117. Should preferably be merged after #120.

Description

This commit bases CordovaError on the popular joyent/node-verror.

We actually use @netflix/nerror, a VError fork for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's ongoing work to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that toString (for errors without a cause argument)
now behaves like the Error default again:

new CordovaError('foo').toString();
// old result: 'foo'
// new result: 'CordovaError: foo'

Here's an overview of available similar packages.

Testing

Added tests that exercised the parts of the VError API that we will be using.

Copy link
Copy Markdown

@brody2consult brody2consult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@raphinesse raphinesse marked this pull request as ready for review December 13, 2019 11:43
@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 13, 2019

Codecov Report

Merging #121 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   88.02%   88.04%   +0.02%     
==========================================
  Files          20       20              
  Lines        1169     1171       +2     
==========================================
+ Hits         1029     1031       +2     
  Misses        140      140              
Impacted Files Coverage Δ
src/CordovaError.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6e8cab...e25f155. Read the comment docs.

@raphinesse
Copy link
Copy Markdown
Contributor Author

FYI: My plan is to merge this after the big refactor PRs have all landed

This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
@raphinesse raphinesse merged commit fe6a73f into apache:master Mar 14, 2020
@raphinesse raphinesse deleted the error-cause branch March 14, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants