How should replaceAll behave if searchValue is a non-global RegExp?

[jridgewell]

Related to, but different from #8. #8 wants it to auto-convert a non-global regex to a global regex. I think we should accept regex searchValues, but not convert them to a global regex.

String.p.matchAll is setting precedent for "all" methods:

'test'.matchAll(/t/);
// => [ [first t match] ]

'test'.matchAll(/t/g);
// => [ [first t match], [second t match] ]

[AmeliaBR]

As much as I generally support consistency, the problem is that the two proposals use "All" for different concepts. In matchAll, it refers to returning all the capturing groups for each match, with the number of matches determined by the usual rules. But in this proposal, "All" refers to replacing all the matches instead of just the first one.

I can't think of a better naming convention. But it does mean that you'll no longer be able to polyfill your function with a simple split + join sequence, without some conditionals. (Why didn't I think of s.split(re).join(re) when I was fighting with global RegExp state???)

[ljharb]

In matchAll, though, if you pass a non-global regular expression, it obeys it, and only yields one match - similarly, if you pass a non-global regex to replaceAll, it seems consistent to only replace one match.

[mathiasbynens]

If we follow matchAll, I suppose replaceAll would then behave the same as replace, except for the case where searchValue is a string (which is the only new functionality we want to enable through this proposal). (Well, that, or someone mucked with @@replace/@@replaceAll). In that case, we could avoid having to copy the RegExp instance (which is expensive) + the new @@replaceAll symbol altogether.

[jridgewell]

In that case, we could avoid having to copy the RegExp instance (which is expensive) + the new @@replaceAll symbol altogether.

I think that's a good end result, it also makes it very easy to implement. 😃

[psmarshall]

I agree with @AmeliaBR in that I'd prefer non-global regexps to be changed to global ones. I kind of read it as 'replace all occurrences that match this thing' (but the thing (a regex) only matches once if it isn't global, haha tricked you!). I also agree that the 'all' refers to a different thing.

I also agree with @mathiasbynens that the point of this proposal was to add functionality where searchValue is a string, so it is kind of more 'minimal' not to change anything else.

[AmeliaBR]

If replaceAll with a RegExp isn't actually going to do anything different from replace with the same parameters, it might be better to stick with the original plan of not supporting RegExp parameters at all. (I have no opinion on whether it should throw if a RegExp is used, versus silently coerce to string.)

[ljharb]

Just like how match and matchAll both accept the same kinds of things, replaceAll and replace need to accept the same kind of things. Since replace accepts regexes, it's unlikely replaceAll will be able to achieve consensus without also accepting regexes.

[mathiasbynens]

Since replace accepts regexes, it's unlikely replaceAll will be able to achieve consensus without also accepting regexes.

+1 I actually tried to advocate for that the first time I presented this proposal (Nov 2017) and it was explicitly shut down. So yes, throwing for RegExp arguments is not an option for TC39.

[schuay]

It seems there's some confusion about the 'All' in 'replaceAll' and 'matchAll'. That's unfortunate, and why I argued against the - IMHO unintuitive - name 'matchAll' back then.

People have already said in this thread that 'All' refers to totally different concepts in these 2 methods. That's why I don't think it makes any sense to try and reach some sort of consistency between them.

replaceAll is the global variant of replace. It finds and replaces all occurrences of searchValue with replaceValue. Just like what the 'replace all' button in any text editor does. Adding any non-global behavior there sounds pretty crazy to me ;)

I agree with @AmeliaBR in that I'd prefer non-global regexps to be changed to global ones. I kind of read it as 'replace all occurrences that match this thing' (but the thing (a regex) only matches once if it isn't global, haha tricked you!). I also agree that the 'all' refers to a different thing.

+1.

I also agree with @mathiasbynens that the point of this proposal was to add functionality where searchValue is a string, so it is kind of more 'minimal' not to change anything else.

I think what we are currently proposing (add a @@replaceall symbol, make all regexes global) is the only & simplest solution that is consistent with user expectations (that arise due to the similar naming as String.p.replace).

[ljharb]

Given the committee’s reaction to matchAll auto-g-ing regexes (its original behavior), i expect replaceAll would end up in the same place - where “all” applies to all the places matched, and a non-global regex matches 0 or 1 times.

[mathiasbynens]

During offline discussions at the last TC39 meeting I've come back around on this; replaceAll and matchAll both have All in the name, but it indeed has a different meaning. Trying to force "consistency" between the two might not be the best design.

I like @schuay's example of "replace all" functionality in text editors; it would be very surprising if that'd only replace a single instance if "RegExp mode" is enabled.

[ljharb]

Text editors treat the input as a pattern, and implicitly auto-g for “find all” and “replace all” - but they typically have a non-global regex for “find” and “replace” - I’m not sure that comparison can really hold here. If the user has explicitly provided a non-global regex, then i think they’ve explicitly chosen non-all semantics.

[mathiasbynens]

If the user has explicitly provided a non-global regex, then i think they’ve explicitly chosen non-all semantics.

They already explicitly choose the "all" semantics by using replaceAll instead of replace. It doesn't get more explicit than that, IMHO.

[ljharb]

While i agree with that philosophically, the committee explicitly chose to not go with “choosing all is an explicit choice for g” for matchAll, and i don’t see why replaceAll would be any different.

[getify]

Could we rename replaceAll(..) to replaceEvery(..) so that we don't have the (extremely likely!) confusion/conflation between the two different usages of the word "all"?

Then replaceEvery(..) could act like a global find-n-replace in text editors, meaning that it always replaces every match, regardless of the regex flag.

[rkirsling]

Given the non-analogy of this feature with matchAll and the expectation for a regex parameter to be supported, it really does seem dangerous to proceed under the current naming.

I agree that replaceEvery could be a decent way to solve that—the developer may wonder why it's "every", but they'd be less likely to assume that its behavior will align with another existing method.

[ljharb]

"every" suggests Array.prototype.every to me, whose short-circuiting predicate behavior doesn't really match what this function does.

[jridgewell]

Does anyone remember why we made matchAll not coerce to a global regex?

[ljharb]

@jridgewell that was my initial design; see here but mostly https://github.com/rwaldron/tc39-notes/blob/def2ee0c04bc91612576237314a4f3b1fe2edaef/meetings/2015-09/sept-23.md#512-proposal-stringmatchall - by adding the g it requires the regex to be observably cloned, and some folks didn't want to do that.

[hax]

I agree for the consistency with match/matchAll, we should not auto-g. But it not meet user expectation , doesn't fix replace gotcha and just make the gotcha worse because of all in the name.

And if the only diff of replace/replaceAll is replacing all string, we'd better go back to the original proposal (only accept string) and change the api name (like substitute, pls revisit old issue: #9).

So I think there are only two reasonable solutions:

1. change api name, use original string-only semantic
2. change matchAll semantic, so matchAll/replaceAll both auto-g (or even throw typeerror for non-global regexp which is better in the perspective of engineering).

[Dan503]

Based on the results of this Twitter poll, it looks like there is an overwhelming support for replaceAll to auto-g

https://twitter.com/AmeliasBrain/status/1146496834275143681?s=19

(I'm massively in favour of auto-g being applied to both matchAll and replaceAll)

[ExE-Boss]

I agree, both should functions auto‑g the RegExp.

[ljharb]

The committee disagreed and matchAll shipped with the current semantic of obeying the regex it’s given - that ship, i think, has sailed.

[Dan503]

matchAll shipped with the current semantic of obeying the regex it’s given

😢

It is still early days, it is possible that developers either aren't using it yet or they are only using it with the g flag enabled.

The only way updating this functionality will break a site is if the developer has used matchAll with a non-global regex. If they are doing that, why would they use the far less supported matchAll when they can use match?

Basically what I'm saying is that I think it is unlikely that a developer has used matchAll with a non-global regex anyway. Also, if they have, it's most likely a bug in their code featuring undesirable behaviour.

A warning can be issued in the console if a non-global regex is being used saying that the functionality is changing and to use match instead.

I know it isn't desirable but I have seen something handled in this way once before. I can't remember what it was for though 😖

[ljharb]

For a consistent api - as in, right now you never need to use match, only matchAll.

The console isn’t part of the language.

[Dan503]

That is a fair point. Code can be made cleaner by using matchAll if sometimes the regex is global and sometimes it's not.

No need to check if the global flag is enabled on the regex to switch between match and matchAll.

...Is it even possible to check for a global flag on a regex? 🤔

[ljharb]

@Dan503 yes, with .global or with .flags.includes(‘g’).

[AmeliaBR]

In addition to the direct results to the poll, I've been reading all the comment replies. Some thoughts based on those:

A number of people bring up @mathiasbynens's original idea that replaceAll should only apply to strings, & a RegExp should throw an error. (I'm sorry I didn't think to include this option in the poll itself!) At this point, I would prefer this over having replaceAll with a RegExp have the exact same behavior as replace with a RegExp. At least this means that an author will find out sooner rather than later that their non-global regex isn't going to return the result they want! (I'd still prefer the auto-g behavior.)

There was also an interesting suggestion that, instead of a new method, this functionality could be achieved with a new options parameter to replace. The options would have the same structure as the flags parameter to a RegExp constructor, and would override flags on the input RegExp, or make a string pattern behave like a RegExp that matches that literal string but with these flags. (NOTE, this is not the same as passing that string into a RegExp constructor: a pattern "." would still only match a literal period.) Not only would this support global replace with string input pattern, but also case-insensitive replace.

Based on all the discussion, my prefered order of outcomes are:

1. Follow @getify's suggestion, and change the name of this proposal to replaceEvery or something else that doesn't imply a similarity with matchAll. Then, make it truly replace every match, so that s.replaceEvery(p, r) is the same as s.split(p).join(r).

2. Don't add a new method, instead add a flags option to replace as described above.

3. Keep the replaceAll name, but the replace-every behavior, and don't worry about the comparison with matchAll.

4. Make replaceAll only accept strings & throw for RegExp.

5. Let replaceAll accept RegExp, but the result is the same as replace with the same RegExp parameter.

[ljharb]

To me, “all” for a non global regex i expect to be a list of 1.

[getify]

@ljharb that's like a word in some prose having an asterisk/footnote on it.

<joke>could we just rename it to matchAll*() and replaceAll*()?</joke>

[ljharb]

It’s how querySelectorAll works when it only finds one match.

[getify]

It’s how querySelectorAll works when it only finds one match.

That's entirely different. querySelectorAll(..) would return every possible matching element if there were any more than one.

The analog here would be if you had to do querySelectorAll( "...", true ) where true meant "no, really, ALL, EVERY SINGLE ONE, not just the first one."

[ljharb]

A non-global regex can only have one possible matching element (for a given lastIndex value) - it's explicitly part of the contract of a non-global regex.

[getify]

In CSS, everything is matched, unless you go out of your way to restrict it. A selector like p gets all possible <p>s, whereas you have to do p:first to get querySelectorAll(..) to return only the first one.

The default behavior in the absence of a g on a regex has a different DX than the default behavior in the absence of a :first on a CSS selector; they're literally opposite. IMO the latter matches what I expect from the word "All", whereas the former doesn't match at all what I expect from "All".

What about flipping this around... having replaceAll(/something/) matches everything, but replaceAll( /something/ , true ) where true means "only the first one"? Or maybe it's a number instead of a boolean, so you can restrict how many it replaces? That way, if I leave off (or forget) the true (or number), I get a default where "All" means "everything", but if I want to -- like in CSS selectors -- restrict my "search", I add an additional thing to restrict it.

[getify]

Imagined scenario:

1. Text editor has a "Find-and-replace" feature you activate from a menu... in the dialog box, it has a checkbox for "replace all", which defaults unchecked. If you don't check it, the replace only replaces one item. If you do check it, the replacement happens globally.

2. Later, text editor (in response to user requests) decides to add another hotkey they call "Find-and-replace-all". It just pops up the same dialog box as the original menu item.

3. You might assume it would at least default that "all" checkbox to on if it was launched from the "all" hotkey. But... nope. Same default-to-off checkbox.

Can you see why that would be confusing to users?

When the users complain to the author of this text editor, they just respond, "Look, if you want it to replace all, you have to remember to check the checkbox. If you forget, you only get a single replacement. Doesn't matter what the feature name was that you invoked."

---

It is a very hard sell to explain to learners:

"The difference between replace(..) and replaceAll(..) is only with string arguments. With regexes, these two functions do the same thing -- regardless of the name. No, don't think about the 'All', that's only for strings! When you use a regex, you gotta remember that little 'g' flag to control the behavior. No, listen, I told you, the 'All' doesn't mean anything when you're using a regex, only for strings. Argh!"

[ljharb]

Yes, i understand that regexes carrying the state for “global” creates this problem - editors (and some languages) don’t have such a flag usually, so they have to slash get to rely on other UI/API to do so.

Unfortunately, in JS, the presence of the g flag would make any such UI redundant - which means that “one match” vs “all matches” would be a very confusing and inconsistent and redundant thing to be specified by API naming.

That this means that the word “all” might confuse those who completely reasonably expect it to mean “all matches” is imo an unavoidable consequence of the g flag existing, and trying to half-“fix” that would make things worse. (matchAll isn’t really about all matches, which match already provides, it’s about all matches’ match objects, which match doesn’t provide when the regex is global).

[rkirsling]

Regarding my earlier suggestion of Atomics.wake being a possible precedent for renaming:

While I disagree that renaming is inherently riskier than changing functionality (for precisely the reason that @hax gave above), it seems that this isn't a usable precedent here anyway. Going back to the notes, I had forgotten that it was the unpublishing of the Atomics feature as a Spectre/Meltdown mitigation that created a unique opportunity to rename a method that was already in the spec. Apologies for the ruckus.

[SanderElias]

We were discussing this and I wanted to voice my opinion on this.
those came to table:

  "aaa".replaceAll(/a/, 'b') 

what's the expeted result?
The correct way is throwing an error if you want to use a regex, don't use replaceAll, that makes no sense.
A slightly less ugly outcome would be: bbb as that would reflect developers expectations.
Other options are not explainable to a common developer. Sometimes clarity trumps purity.

Edit: I really feel this should throw on any regex. As there is already the normal replace that can handle regex, this one isn't adding any value. And more importantly, it makes it harder for browser engines to optimize. Throwing only for non-global regex, and allowing global ones is eventually my 3rth preference, but again, this is making it even fuzzier for devs. allowing a normal regex to use it as a global really makes more sense.

[getify]

@SanderElias Agreed, I listed earlier the same top two "best options" as you, but mine were flipped in preference order. I can't see any other option being better (or even acceptable) than one of those two.

EDIT: to clarify, for throwing an error, I think a non-global regex should throw, but a global regex should be accepted.

[getify]

It was claimed earlier in this thread that replaceAll(..) couldn't treat a non-global regex as a global regex without it being (undesirably) observable. But is that entirely true?

This is a naive, poorly performing approach, and maybe/probably I am missing some details (?), but... would something like this be possible?

function replaceAll(str,re,...args) {
   if (re.global) {
      return str.replace(re,...args);
   }
   else {
      let newStr = str;
      let prevStr;
      // repeatedly apply the one non-global replace RE
      // against the string, until no more changes
      // occur
      do {
         prevStr = newStr;
         newStr = prevStr.replace(re,...args);
      }
      while (newStr != prevStr);
      return newStr;
   }
}

var str = "Hello World";
var re = /l/;

replaceAll(str,re,"L");   // HeLLo WorLd
replaceAll(str,re,x => x.toUpperCase());   // HeLLo WorLd

[ljharb]

What’s the lastIndex of re after each call to replace?

[getify]

AFAIK, lastIndex doesn't get set on non-global regexes, so it would be undefined. But since it's actually a non-global regex that's just being repeatedly applied, that would seem the expected and natural behavior.

[ljharb]

I think it would on a sticky one, but fair point there.

[getify]

I think it would on a sticky one

Ahh, forgot about stickies.

var s = "abababab";
var re = /a./y;

replaceAll(s,re,x => { console.log(`lastIndex: ${re.lastIndex}`); return x.toUpperCase(); });
// lastIndex: 2
// lastIndex: 4
// lastIndex: 6
// lastIndex: 8
// "ABABABAB"

To me, this seems expected/reasonable. But it would (unfortunately) diverge from how replace( / .. /yg, .. ) works:

var s = "abababab";
var re = /a./yg;
s.replace(re,x => { console.log(`lastIndex: ${re.lastIndex}`); return x.toUpperCase(); });
// lastIndex: 0
// lastIndex: 0
// lastIndex: 0
// lastIndex: 0
// "ABABABAB"

Is that divergence a deal breaker? Weighing all various downsides discussed so far, I wouldn't put that high on the list.

Basically, we could just document that "All" in the case of non-global regexes means, "repeatedly applied individually until all possible replacements are complete. This is different, and may perform differently, than just passing a global-flagged regex."

[getify]

Upon further reflection, there's serious issues with my "repeatedly apply..." suggestion that are probably deal breakers.

For example:

var re = /./;
var str = "abc";
replaceAll(str,re,"x");   // will run forever, since `re` matches the replacement

I think we would have to explore if the JS engine could somehow -- internally only -- update the effective lastIndex property as it was doing the loop?

Maybe it could like clone the original regex once, and actually use a temporary regex where it could update the lastIndex? Maybe the spec could change lastIndex to be an internal slot as well as a public property, that are normally updated at the same time, but in this special case, during the algorithm, update only the slot and not the property.

I dunno. Just pondering possible ways to hack this. If there's absolutely no way to do anything like that, this idea is probably DOA, and I'd go back to saying this case should just throw an exception.

[Edit]: Sorry for the explorative noise. Here's a much more complex polyfill that works around these sorts of problems. Ugly, but I think it works: https://gist.github.com/getify/f9df482f89ac8cdc17f339c81da74c3d

[SanderElias]

@getify You seem to be documenting technical reasons why it is a good idea to throw on all regex ;)

[iamakulov]

One more point I don’t see anyone mentioning. This implementation is future-compatible:

'aaa'.replaceAll(/a/, 'b') // exception thrown

whereas this one is not:

'aaa'.replaceAll(/a/, 'b') // 'bbb'

If we make replaceAll throw and then learn that developers find this behavior confusing, we’ll be able to start accepting regexes later. If we make replaceAll accept regexes and then learn that this implementation is confusing, we won’t be able to revert to throwing.

[Splaktar]

Good discussion here!

TypeDefs for String.prototype.replace

/**
  * Replaces text in a string, using a regular expression or search string.
  * The original string is left unchanged.
  * @param searchValue A RegExp object, literal, or string to search for.
  * @param replaceValue The string that replaces the substring specified by the specified searchValue parameter. A number of special replacement patterns are supported.
  * @returns A new string with some or all matches of a pattern replaced by a replacement.
  */
replace(searchValue: string | RegExp, replaceValue: string): string;

/**
  * Replaces text in a string, using a regular expression or search string.
  * @param searchValue A RegExp object, literal, or string to search for.
  * @param replacer A function that returns the replacement text.
  * @returns A new string with some or all matches of a pattern replaced by a replacement.
  */
replace(searchValue: string | RegExp, replacer: (substring: string, ...args: any[]) => string): string;

I've read through this whole thread and my suggestion would be for String.prototype.replaceAll to throw if passed a RegExp object.

I find the following to be clear and provide the desired convenience:

/**
  * Replaces all text matches, of a search string, in a string with the replacement string.
  * The original string is left unchanged.
  * @param searchValue A string to search for.
  * @param replaceValue A string containing the text to replace for every successful match of searchValue in this string.
  * @returns A new string with all matches of searchValue replaced by replaceValue.
  */
replaceAll(searchValue: string, replaceValue: string): string

As noted above, I agree that it would be easier to change this to the following later if there was demand for it:

replaceAll(searchValue: string | RegExp, replaceValue: string): string

I also agree that String.prototype.replaceAll should not be designed to replace String.prototype.replace. I see it as a convenience method and optimization path, not a replacement or attempt to deprecate String.prototype.replace.

[mathiasbynens]

Re-posting some valuable links that @ljharb dug up w.r.t. the String.prototype.matchAll behavior:

• stage 1 for matchAll: https://github.com/rwaldron/tc39-notes/blob/def2ee0c04bc91612576237314a4f3b1fe2edaef/meetings/2015-09/sept-23.md#512-proposal-stringmatchall
• the change was actually made here in response to community feedback: Respect [absence of] flags semantics of regular expression proposal-string-matchall#6 (comment) / Refactor the spec proposal-string-matchall#17 and then it was approved as part of stage 2 signoff

[mathiasbynens]

We're going with the solution in #24: replaceAll now throws if searchValue is a non-global regular expression (and matchAll is changed accordingly). This will be discussed at the upcoming TC39 meeting.

[littledan]

Not sure if you're still looking for feedback here, but I support somehow making sure that replaceAll does not act non-globally. Either implicitly adding the g flag, or throwing an exception, sounds like a good path to me. I think matchAll's behavior is was pretty surprising and not very useful, and I expressed this as we were discussing it the proposal TC39 and on the issue tracker.

[Zarel]

@littledan I'm not sure if you missed the previous comment or I'm misinterpreting you, but that's already happened: They decided to choose the "throwing an exception" option.

[littledan]

@Zarel I'm expressing agreement with that comment. I was asked to give feedback on the Stage 3 review issue.