Marak / colors.js

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

Add multiline handling functionality #220

Closed jpike88 closed 6 years ago

jpike88 commented 6 years ago

Allows multiline logs to have each line styled properly.

DABH commented 6 years ago

Thanks for the PR! Seems like some basic functionality has been broken though as tests are failing. Can you please investigate and see what changes need to be made in order to get all the tests passing again? Once everything passes, I'm happy to take a closer look and see if we can merge this.

DABH commented 6 years ago

Also, please try to not make any style changes (adding/removing spaces, etc.). We will be revisiting style in the not-too-distant future but for now let's please match what's there.

jpike88 commented 6 years ago

@DABH good to go

DABH commented 6 years ago

Thanks for updating. This is looking pretty good now. My only concern is, how much does this affect performance for the most common use case (single-line string)? And is there any way we can mitigate any such performance risks? First thought was sticking an if(str.indexOf('\n')) in front of the new line but that isn't particularly fast. It's also possible that there is no silver bullet here and that we simply have to pay this price to make multiline strings work -- if so, ok, but I want to think a bit more (feel free to research/comment as well).

Also worth reading https://stackoverflow.com/questions/9750338/dynamic-vs-inline-regexp-performance-in-javascript and https://jsperf.com/regexp-indexof-perf/24 , there might be a more efficient approach than constructing that new RegExp every single time (that worries me a bit).

jpike88 commented 6 years ago

I could make this an option and disable it by default.

jpike88 commented 6 years ago

@DABH thoughts?

DABH commented 6 years ago

I'll take a look, give me a day or two ;)

jpike88 commented 6 years ago

https://gist.github.com/jpike88/868b7912354907300ec790e34104557d

jpike88 commented 6 years ago

My PR gives me 636ms,

Where the original code says 52ms. Definitely a difference there.

jpike88 commented 6 years ago

I have done more experiments on a more up to date benchmarking script:

https://gist.github.com/jpike88/7c48d33111dc5f6d1e879792dfe3c6d3

I've tried split(/[\r\n]+/).join and that was slower... also tried several other combinations.

The best solution I think is to do an indexOf first, then use that flag to apply newline joins.

This keeps things as optimal as can be, but the truth is, I think the expectation is that if you're punching out heaps of multi-line output (with colors included), I doubt performance should be as much of a factor. This rule even somewhat holds for using stdout in general right?

The single indexOf check increases the fastest times from about ~51ms to ~64ms, I think that shouldn't pose too much of a problem.

DABH commented 6 years ago

Nice, thanks for looking into this. Ha, very glad we checked performance, yikes.

I'm just about ready to go here. The only thing I'd ask is if you could add some test cases? I'm thinking:

jpike88 commented 6 years ago

done, no worries!

jpike88 commented 6 years ago

requesting a release on the pr getting approved, when you get a moment

DABH commented 6 years ago

Thanks, this got merged and I just published a new release (1.2.2). Thanks again for your patience and contributions, it's greatly appreciated!