Marak / colors.js

get colors in your node.js console
https://github.com/Marak/colors.js
Other
5.17k stars 446 forks source link

Update supports-colors.js #172

Closed paladox closed 6 years ago

DABH commented 6 years ago

(1) is this up to date still? (2) can we not add dependencies (has-flag)? Just fold that one into the repo as well I guess?

Thanks!

DABH commented 6 years ago

(and, if this is the PR we end up merging, can you merge in origin/develop so CI passes?)

paladox commented 6 years ago

@DABH not sure. I havent worked on this in 2 and half years :)

DABH commented 6 years ago

So amazed you replied after all this time :) If you're still interested in helping out, would be awesome to work on this change... this is basically one of two major changes that has to happen I think to make the lib work for the long term future ;)

paladox commented 6 years ago

@DABH i think i've updated correctly :).

DABH commented 6 years ago

Can we not add dependencies (has-flag)? Just fold that one (it's only one extra .js file there) into the repo as well I guess (put in same folder as supports-colors.js)? Thanks again!!

paladox commented 6 years ago

@DABH once i add that file, how do i load it though?

DABH commented 6 years ago

var hasFlag = require('./has-flag.js');??

paladox commented 6 years ago

thanks.

DABH commented 6 years ago

(CI tests should pass now -- if there are errors, it's with this PR and we need to fix them :) )

paladox commented 6 years ago

@DABH done.

paladox commented 6 years ago

Actually we will need to support nodejs 4.x+ see https://github.com/chalk/supports-color/commit/796903f261f88b9e17294bff8cd25a528289c7b4#diff-354f30a63fb0907d4ad57269548329e3

paladox commented 6 years ago

test fails for nodejs 0.x which is expected see https://github.com/chalk/supports-color/commit/796903f261f88b9e17294bff8cd25a528289c7b4#diff-354f30a63fb0907d4ad57269548329e3

DABH commented 6 years ago

We could instead just use function() and var for now and maintain compatibility? :) That way the bug fixes provided by this PR would still help people who for some reason are on super legacy Nodes... what do you think?

DABH commented 6 years ago

(Then we could switch to modern ES syntax for the next major release, or something)

paladox commented 6 years ago

@DABH it also includes other things like Numbers. Is Numbers is an old es version? Support by nodejs 0.10?

DABH commented 6 years ago

Since ES1 I think :) https://www.w3schools.com/jsref/jsref_number.asp give it a try and lmk though, let's see what Travis says :)

DABH commented 6 years ago

oh, let is also an issue, we can make that var also (the modern ES dev inside me cries a little but ok...)

paladox commented 6 years ago

@DABH it seems to have other es6 functions witch im not sure how to convert. Should i go back to my old version the one where you first commented? As it seems we should update to the latest version when we drop nodejs 0.x support?

paladox commented 6 years ago

@DABH done, I've reverted to my prevous version. We should after we release a new version. remove nodejs 0.x support.

DABH commented 6 years ago

Let’s keep going with trying to make a version that’s compatible with everything. That ES function syntax is just shorthand – for example, I see a function that takes an argument and return that ES function syntax is just shorthand — if we expand it out and/or refactor it into a separate helper function, it should be compatible. I can help with syntax and take a look later if you’re not sure.

paladox commented 6 years ago

@DABH done. I found https://babeljs.io/repl/

paladox commented 6 years ago

@DABH passes.

DABH commented 6 years ago

Ah, sweet! I’ll review again in a bit. Thanks!!

DABH commented 6 years ago

No warnings on any of the builds, this is a good sign :) I'll look more closely, try to break things etc., but if I can't find any way in which this is bad, we can probably include this in 1.2...

paladox commented 6 years ago

thanks :)

DABH commented 6 years ago

Ok, I'm going to merge this for now (!). I am going to make a few small changes related to this on my own (that I'll push straight to develop). There are probably some additional tests that could be added -- in fact I might just borrow some of the tests from supports-colors and has-flag. But overall this seems like a strict improvement, fixes critical issues with the package, etc., so there is no good reason not to merge this. Thanks again!

paladox commented 6 years ago

Thanks :)

paladox commented 6 years ago

@DABH could the default branch be changed to develop please?

DABH commented 6 years ago

No :) But I will publish a pre-release version soon, though, so you can use "colors@next" in your package.json if you want the latest version (importantly, though, it won't install that version unless you explicitly specify it; good release management stuff etc....)

DABH commented 6 years ago

(Published, see readme for the repo)

paladox commented 6 years ago

Thanks :)