-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix] remove verror dependency from lib code #88
Conversation
src/error.js
Outdated
class DetailedError extends Error { | ||
constructor(details, message) { | ||
super(typeof details === 'string' ? details : message) | ||
|
||
class HarToK6Error extends VError {} | ||
class InvalidArchiveError extends HarToK6Error {} | ||
class UnrecognizedError extends HarToK6Error {} | ||
if (typeof details === 'object') { | ||
Object.assign(this, details) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error mimics the verror
options object. We pass a name
to the error for extra detail. I wasn't 100% certain that we always passed that object though, so I added some type checking for extra safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass { name: string }
in every call to InvalidArchiveError
and UnrecognizedError
.
const { VError } = require('verror') | ||
|
||
class CommandLineError extends HarToK6Error {} | ||
class CommandLineError extends VError {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only care about the added functionality from verror
when it's a CommandLineError
, so I just moved the dependency up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Shaves off a good chunk of the bundle. 🎉
Next up moment
? 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've re-introduced HarToK6Error
for backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
The
verror
package used insrc/errors.js
has a dependency on node built-in modules, which means that the library code requires a bunch of node-polyfills to work in the browser. This PR removes that dependency by only using theverror
module inbin/har-to-k6
.