-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Make properties enumerable + fix TS typings #18
Conversation
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.
Thanks Marvin, glad you like it! 🙌 Thanks for the PR, too 🙇
✅Not having enumerable was an oversight, oops.
🤔 I think the types are now wrong, differently. I don't use TS, so forgive me if this is incorrect, but it looks like there will be a new enabled
property per-Color, even though enabled
is only a top-level key. Is this right?
@lukeed Thanks for your review 👍 Good catch! You're right regarding the |
727003e
to
a80c2a5
Compare
* import kleur = require('kleur'); | ||
*/ | ||
export let enabled: boolean; | ||
declare let kleur: Kleur & { enabled: boolean }; |
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.
Hah, cheeky! Have never seen this before 👏
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.
Thank you~!
Thanks for making another awesome library! Have used it in a few personal projects so far and love it 👍 While working with it I'm wondering if you're open to these two suggestions:
1) Make properties enumerable
A simple
console.log(kleur)
would previously only showenabled
property as available which might be confusing to newcomers. With this change it would list all available color properties.2) Fix TypeScript typings
TS typings for
commonjs
modules are not as straightforward as fores6
based libraries. If the libraries code is exported viamodule.exports = myLib
one cannot declare it asdefault
exports as this would create an ambiguity in regards to named exports.This PR changes that to the recommended way of changing the declaration to
export = myLib
(verified locally).