Feature: build and nits #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added all contributors and a readme starter to have a decent place as a homepage for this repo. The readme follows the kibibit readme template. changed license date to 2020 instead of 2019
almost the same behavior and jest has some extra features. Also, this is more uniform with the testing tools we use with other repos so should be easier to understand when contributing between repos
Changed the name assignment to take the name from the constructor. This should make it so that whenever an error extends KbError, the name will be initialized by the super call. This will be simpler and make sure it not depends on people forgetting to do this.name = '...' :-) https://javascript.info/custom-errors#breadcrumb-2:~:text=Now%20custom%20errors%20are%20much%20shorter%2C,%3D%20...%22%20line%20in%20the%20constructor.
- Removed some type checks. For example: We're not actually testing the message 'type', so it can be something else than a string. for example, passing an array is totally acceptable as a message. - Added some extra checks for 'should be extendable'
locally, and as a github action
didn't add a github action for this one. maybe later we'll add this: https://github.com/wagoid/commitlint-github-action
also removed the contributors shields since it doesn't fit the format
thatkookooguy
force-pushed
the
feature/build-and-nits
branch
from
e2ab524
to
335e691
Compare
thatkookooguy
commented
Nov 11, 2020
| @@ -5,6 +5,6 @@ module.exports = class kbError extends Error { | |||
| */ | |||
| constructor(message) { | |||
| super(message); | |||
| this.name = 'kbError'; | |||
| this.name = this.constructor.name; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
The problem with this approach is when things get uglified and you lose the name.
I have no problem with this change as we're currently not uglifying, but we need to keep this in mind.
There was a problem hiding this comment.
Good Point!
But right now, I think this doesn't support browser as-is since it doesn't support RequireJS (because of the module.export = part).
Even so, What you're worried about is easily fixable imo.
- we can always just use it as you intended when requiring on client-side, setting the name in the extended class
- Most minification\uglification libraries have support for this kind of exclusions.
You can just not minify class names. That might be too project-wide but it's an option :-)module.exports = { entry: './kb-error.js', output: { filename: 'kb-error.min.js', path: path.resolve(__dirname, 'dist'), }, optimization: { minimize: true, minimizer: [ new TerserPlugin({ terserOptions: { mangle: { keep_classnames: true } } }) ], } };
But then again, using:
class FooError extends kbError {
constructor(message) {
super(message);
}
}should work without modification (node.js for example)
and:
class FooError extends kbError {
constructor(message) {
super(message);
this.name = 'FooError';
}
}should work fine in the browser with this implementation
ZimGil
approved these changes
Nov 11, 2020
| @@ -5,6 +5,6 @@ module.exports = class kbError extends Error { | |||
| */ | |||
| constructor(message) { | |||
| super(message); | |||
| this.name = 'kbError'; | |||
| this.name = this.constructor.name; | |||
There was a problem hiding this comment.
The problem with this approach is when things get uglified and you lose the name.
I have no problem with this change as we're currently not uglifying, but we need to keep this in mind.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should release versions to npm once this commit is in (this will commit the github action as well)
If this pull request is too big, I can split it up into a couple of pull requests if you want.
TL;DR (I explained my changes in the description of each commit):