Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

Throw if searchValue is a non-global RegExp #24

Merged
merged 3 commits into from
Sep 23, 2019

Conversation

mathiasbynens
Copy link
Member

One of the potential solutions to #16. (If we decide to go this route, we'd want to change matchAll accordingly.)

@mathiasbynens
Copy link
Member Author

@ljharb Any opinions re: the spec.html changes?

@mathiasbynens
Copy link
Member Author

Per your LGTMs, I've added the corresponding matchAll changes as well.

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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I don't have a strong preference tbh. Any opinions? @schuay @ljharb

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@mathiasbynens mathiasbynens merged commit b55f003 into master Sep 23, 2019
@mathiasbynens mathiasbynens deleted the throw-on-non-global-regexp branch September 23, 2019 06:57
zloirock added a commit to zloirock/core-js that referenced this pull request Sep 28, 2019
zloirock added a commit to zloirock/core-js that referenced this pull request Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants