async-replace icon indicating copy to clipboard operation
async-replace copied to clipboard

Replaces first instance of matched string, not the actual matched regular expression

Open Conduitry opened this issue 9 years ago • 0 comments

This is a bit tricky. I actually constructed this example after taking a quick look through this code, to confirm that what I thought was a bug was indeed a bug.

Suppose you have the string 'xaxb' and you're doing a replacement based on the regex /x(?!a)/g. That is, we're looking for all x that aren't immediately followed by an a. So this should match the second x and not the first x. If we use regular old .replace() we see that: 'xaxb'.replace(/x(?!a)/g, 'y') is 'xayb'.

If we do the same replacement with this library, though, the result is incorrect: 'yaxb'. When you use the String.prototype.match method to do the regex matching, all this returns to you is the array of matched strings - not where the matches occurred. Later, .indexOf() dutifully finds the first instance of 'x' in the string (which is all it has to go on), and this is eventually replaced. But this isn't what was actually matched or what should be replaced.

To avoid this bug (and also to avoid re-searching for the string, which is inefficient), you could instead use RegExp.prototype.exec, which will tell you, for each match, exactly where it occurred, which might not be the earliest instance of the matched string. This is the approach I took when was implementing my own (promise-based rather than callback-based) function for this, before I found yours.

Do whatever you'd like with this bug report! I'm not actually using your library, nor do I actually need this lookahead support.

Conduitry avatar Aug 27 '16 22:08 Conduitry