Advance to stage 3

[mathiasbynens]

Criteria taken from the TC39 process document minus those from previous stages:

• Complete spec text

https://github.com/tc39/proposal-string-replaceall#specification

• Designated reviewers have signed off on the current spec text

• @msaboff
• @littledan
• @gibson042 (modulo comments to be resolved during TC39 meeting)

• All ECMAScript editors have signed off on the current spec text

• @ljharb

[mathiasbynens]

Would the reviewers have time to take a look before the upcoming TC39 meeting later this month?

In addition to the existing spec text, I'd like to hear your input on this issue: #16

[ljharb]

Existing spec text review:

• i presume StringIndexOf will also be used in replace - could that del/ins change be added as well?

Otherwise, consider me signed off.

Re #16; my preference is to leave it as-is, consistent with replace, match, and matchAll for non-global regexes.

[mathiasbynens]

i presume StringIndexOf will also be used in replace - could that del/ins change be added as well?

Perhaps counter-intuitively, that would actually make replace less clean, and introduce additional steps. Step 7 in https://tc39.es/ecma262/#sec-string.prototype.replace does StringIndexOf and then some:

Search string for the first occurrence of searchString and let pos be the index within string of the first code unit of the matched substring and let matched be searchString. If no occurrences of searchString were found, return string.

I propose we just leave replace as-is.

[mathiasbynens]

@msaboff, @littledan, @gibson042, @zenparsing Could y'all take a look as well please? Keep in mind that #16 is still under discussion.

[msaboff]

The current spec text looks fine modulo the resolution to #16. Assuming that #16 is resolved allowing RegExp for searchValue and we don't auto-g Im signed off..

For #16, I don't want to disallow RegExp in replaceAll. That really seems counter intuitive.

My preferred resolution to #16 is that we auto-g, but I am somewhat swayed by the precedent of matchAll not auto-g'ing. I personally don't think the copying of a RegExp, adding the g flag is a big deal. The "All" in a name (for both matchAll and replaceAll) connotes a repetitive application of an RegExp.

[mathiasbynens]

Thanks for the feedback, @msaboff. FWIW, the auto-g spec PR is here: #20 The other suggested options (e.g. if regexp, throw) are even simpler changes, spec-wise.

[gibson042]

The spec text looks correct for a replaceAll that differs from replace only in discovery and replacement of all substrings matching non-RegExp-ish search input, but I can't shake some nagging issues with that state of affairs. For one, replaceAll feels like the wrong name—it suggests completion of a grid that already includes match, matchAll, and replace, but the meaning of "All" is completely different from its always-include-captures semantics in matchAll, no matter how #16 is resolved (an issue that I consider a Stage 4 blocker, but acceptable at Stage 3 even though I expect the current text to change).

So I think a rename is in order, which opens up the possibility of a signature with behavior that doesn't expect RegExp input (i.e., encouraging authors to use the new method for mass replacement of static strings but the preexisting replace for replacement in which a regular expression pattern controls both matched text and single vs. mass replacement). But special RegExp support could never be added to such a method unless it special-cases input from the start, because String prototype methods make heavy use of coercion (e.g., "score: 100".replace(100n, "💯") === "score: 💯"). And with the recent string prototype methods that expect strings already specified to throw a TypeError when receiving an argument that passes IsRegExp, that would be my preferred approach for this new method as well (unless a clear consensus emerges around #16, including the effects of stickiness).

[ljharb]

Filed #22 as well for a bit of minor review.

[mathiasbynens]

Stage 3 reviewers @littledan and @gibson042, could you please take another look now that #24 is merged? I'll present the proposal in its current form at the upcoming meeting, and (if possible) it would be great to have your reviews before then so we have the option to ask for Stage 3.

Notably: replaceAll and matchAll now throw for non-global RegExps.

[mathiasbynens]

@msaboff Same for you (although I had already checked your review bit earlier) -- PTAL given the recent changes.

And @zenparsing, if you could take a look, that'd be great too :)

[littledan]

Thanks for the review ping. I'm really happy with the semantics here, with #24 landed. LGTM for Stage 3.

[mathiasbynens]

@zenparsing Gentle ping re: your editor review for Stage 3.

[mathiasbynens]

@gibson042 Gentle ping re: your review for Stage 3.

[msaboff]

Here is my review of the latest version.

For both replaceAll and matchAll, Step 2.b. should throw when flags is undefined.

[mathiasbynens]

@msaboff What about null?

[ljharb]

I’d expect both if it’s going to throw, using RequireObjectCoercible

[mathiasbynens]

Done here: #26 PTAL. @msaboff, I’ll merge this once you get back and confirm your intuition re: null.

[mathiasbynens]

#26 is merged. Thanks for the additional round of review, @msaboff!

[gibson042]

I think the language in StringIndexOf should be more formal, as it was in String.prototype.indexOf. Suggestion:

4. Let len be the length of string.
5. Let searchLen be the length of searchValue.
6. If there is at least one integer k such that fromIndex ≤ k ≤ len - searchLen and for all nonnegative integers j ≤ searchLen, the code unit at index k + j within string is the same as the code unit at index j within searchValue, let pos be the smallest (closest to -∞) such integer. Otherwise, let pos be -1.

We should also have a note about the implications for empty searchValue (i.e., that the result is -1 iff fromIndex is greater than the length of string, but otherwise is fromIndex—and in particular that there is a match after the last character of string).

The rest looks good to me, but I think there's a gotcha (albeit one that is shared with String.prototype.replace) regarding sticky global regular expressions. Simplifying a great deal, RegExp.prototype[@@replace] sets lastIndex to 0 for global expressions and then iterates with RegExpBuiltinExec via RegExpExec until that ends with a null result. And RegExpBuiltinExec uses [[RegExpMatcher]] to seek a match at each index in the string starting at lastIndex (or for non-global non-sticky expressions, a substitute value of 0) until a stop condition is reached (sticky expressions allowed only one failure, others running to the end). So the final effect is that while non-sticky global expressions replace every matching substring, sticky global expressions only replace every consecutive match from the beginning—and if the beginning doesn't match, then they don't replace anything at all.

Concretely, we get this potentially surprising behavior:

"No Uppercase!".replaceAll(/[A-Z]/g, "");	// "o ppercase!"
"No Uppercase?".replaceAll(/[A-Z]/gy, "");	// "o Uppercase?"
"NO UPPERCASE!".replaceAll(/[A-Z]/gy, "");	// " UPPERCASE!"

Further, both @@replace and exec can be overridden to opt-out of the start-at-beginning and/or no-missed-matches behavior. I'm not certain that anything should be done here, but it is potentially a footgun for replaceAll to exhibit "replace-from-beginning" behavior, and the decision about that should be intentional.

[mathiasbynens]

@gibson042 Re: sticky, I admit that's a gotcha I hadn't considered before! Nice find. I agree it's unclear if anything should be done about it, but I will present it to the committee so that any decision on this is intentional.

For the StringIndexOf change, your suggestion looks good to me. Do you want to submit a patch?

[schuay]

I think the language in StringIndexOf should be more formal, as it was in String.prototype.indexOf.

Just wanted to point out, that bit came from RegExp.prototype.replace which has an almost identical step 7: https://tc39.es/ecma262/#sec-string.prototype.replace. I agree that less hand-wavey is good though, especially for the corner cases you pointed out.

[schuay]

The rest looks good to me, but I think there's a gotcha (albeit one that is shared with String.prototype.replace) regarding sticky global regular expressions [...]

The behavior for sticky+global replace is indeed weird, but IMHO orthogonal to this proposal. For (global) regexp patterns, replaceAll should stay consistent with @@replace. Anything else would be even more confusing, no?

[gibson042]

For the StringIndexOf change, your suggestion looks good to me. Do you want to submit a patch?

#27

[mathiasbynens]

The proposal advanced to stage 3 during the 2019-10-02 TC39 meeting.