fix: iteratively replace "close" to avoid maximum stack error

[hi-ogawa]

• Closes RangeError: Maximum call stack size exceeded when coloring already colored long string #63

Hi, thanks for the awesome library!

This is one implementation idea I had to avoid call stack error explained in the above issue. I'm not sure if you have any particular reason you've chosen recursive one (perf? code side?), but just to have something to discuss, I'm starting this PR here.

Please let me know what you think. Thanks!

[alexeyraspopov]

Thanks for raising the issue and making a PR for it! Let me briefly go through the code first. It passes the tests so nothing prevents me from merging it. After the merge I'll need to make some tests to see how it affects existing benchmarks and if we need some more of them for this kind of tasks.

[alexeyraspopov]

@hi-ogawa, I did some benchmarking locally, testing out the suggested changes. Unfortunately, there's very clear regression in the main benchmark, that is a lot larger than an error threshold:

main branch        4,798,017 ops/sec
this branch        2,990,548 ops/sec

I assume this is mainly due to extra conditions that the solution requires.

I do like your approach with cycle, so I tried to play around with it. Here's what I ended up with

let formatter =
	(open, close, replace = open) =>
	input => {
		let string = "" + input
		let index = string.indexOf(close, open.length)
		return ~index
			? open + replaceClose(string, close, replace, index) + close
			: open + string + close
	}

function replaceClose(string, close, replace, index) {
	let result = ""
	let cursor = 0
	do {
		result += string.substring(cursor, index) + replace
		cursor = index + close.length
		index = string.indexOf(close, cursor)
	} while (~index)
	return result + string.substring(cursor)
}

This provides following benchmark results, which is even higher than the baseline. Something that I'd expect from using a cycle instead of non-TCO recursion.

main branch          4,777,274 ops/sec
this solution        4,963,734 ops/sec

The striking difference coming from not having extra conditions for certain cases. We still need an extra function which in isolation may have fewer assumptions and provide more straightforward computations.

Let me know what you think about this approach. If you can update your branch with suggested changes, we can run through tests one more time and merge it. I will publish a patch version update after that.

[hi-ogawa]

Very interesting! Right, I was thinking the special casing the first check could have significant impact, but I didn't properly compared with main branch. Thanks for testing with it.

Your code looks good to me and now I updated a branch with your suggestion. Thanks for the review!

[alexeyraspopov]

@hi-ogawa v1.0.1 is now available in NPM.