Throw if searchValue is a non-global RegExp
#24
Conversation
|
@ljharb Any opinions re: the |
|
Per your LGTMs, I've added the corresponding |
| 1. <ins>If _isRegExp_ is true, then</ins> | ||
| 1. <ins>Let _flags_ be ? ToString(? Get(_regexp_, `"flags"`)).</ins> | ||
| 1. <ins>If _flags_ does not contain `"g"`, throw a *TypeError* exception.</ins> | ||
| 1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll). |
There was a problem hiding this comment.
it does seem kind of odd that a non RegExp can have any kind of non “all” behavior it wants, but an actual RegExp is forced to have the proper “all” behavior. Not sure anything can be done about that, but it is an inconsistency introduced by this change.
There was a problem hiding this comment.
I see what you're saying. Still, there's value in making built-ins work well / in unsurprising ways by default, even if for userland subclasses, this is up to the user.
There was a problem hiding this comment.
If we only/mostly care about built-ins, I wonder if makes more sense to move this check into https://tc39.es/ecma262/#sec-regexp-prototype-matchall?
There was a problem hiding this comment.
No strong opinion. One argument for the current spot is that the location is consistent between matchAll and replaceAll. There's no @@replaceAll so we could not consistently move both checks to a RegExp builtin. I'm fine with either way.
There was a problem hiding this comment.
It seems like the motivation/value of moving it inside the symbol methods is so a subclass could override the behavior - ie, could lack the g flag but still be “all” (vice versa is already going to be possible). I don’t see any benefit in allowing regex subclasses to deviate from this rather strong guard that we’ll have decided on - without concrete use cases, i think here is probably a better spot.
…argument per tc39/proposal-string-replaceall#24 and decision from TC39 meetings
…argument per tc39/proposal-string-replaceall#24 and decision from TC39 meetings
One of the potential solutions to #16. (If we decide to go this route, we'd want to change
matchAllaccordingly.)