Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #471 #745

Merged
merged 4 commits into from
Jan 10, 2020
Merged

fixes #471 #745

merged 4 commits into from
Jan 10, 2020

Conversation

mattclough1
Copy link
Contributor

@mattclough1 mattclough1 commented Jan 9, 2020

Fixes #471 in Safari. Not sure if this falls within the scope of core-js or not since the behavior of $n when n > m is supposed to be implementation-defined, which I think means Safari can do whatever they want?

@@ -28,6 +28,14 @@ var REPLACE_KEEPS_$0 = (function () {
return 'a'.replace(/./, '$0') === '$0';
})();

// Safari <= 13.0.3(?) substitutes nth capture where n>m with an empty string
var REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE = (function () {
if (RegExp.prototype['Symbol.replace']) {
Copy link
Owner

Choose a reason for hiding this comment

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

Symbol.replace is not a string. Use wellKnownSymbol('replace').

@zloirock
Copy link
Owner

zloirock commented Jan 9, 2020

I didn't explore the problem, I'll dig it a little later.

@zloirock
Copy link
Owner

zloirock commented Jan 9, 2020

Not sure if this falls within the scope of core-js or not since the behavior of $n when n > m is supposed to be implementation-defined, which I think means Safari can do whatever they want?

In the spec I see:

The nth element of captures, where n is a single digit in the range 1 to 9. If n ≤ m and the nth element of captures is undefined, use the empty String instead. If n > m, no replacement is done.

So yes, seems it should be fixed. Also, in Safari TP it works correctly.

@mattclough1
Copy link
Contributor Author

mattclough1 commented Jan 9, 2020

In the spec I see:

The nth element of captures, where n is a single digit in the range 1 to 9. If n ≤ m and the nth element of captures is undefined, use the empty String instead. If n > m, no replacement is done.

Ah, shoot, yeah I was looking at an older spec 😕 Well I guess that just makes it a Safari bug then!

@@ -28,6 +28,14 @@ var REPLACE_KEEPS_$0 = (function () {
return 'a'.replace(/./, '$0') === '$0';
})();

// Safari <= 13.0.3(?) substitutes nth capture where n>m with an empty string
var REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE = (function () {
if (RegExp.prototype[wellKnownSymbol('replace')]) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to cache wellKnownSymbol('replace') as a constant, for example, REPLACE.

// Safari <= 13.0.3(?) substitutes nth capture where n>m with an empty string
var REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE = (function () {
if (RegExp.prototype[wellKnownSymbol('replace')]) {
return RegExp.prototype[wellKnownSymbol('replace')].call(/./, 'a', '$0') === '';
Copy link
Owner

Choose a reason for hiding this comment

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

/./[REPLACE]('a', '$0')

@@ -75,7 +83,9 @@ module.exports = function (KEY, length, exec, sham) {
if (
!DELEGATES_TO_SYMBOL ||
!DELEGATES_TO_EXEC ||
(KEY === 'replace' && !(REPLACE_SUPPORTS_NAMED_GROUPS && REPLACE_KEEPS_$0)) ||
(KEY === 'replace' && !(REPLACE_SUPPORTS_NAMED_GROUPS && (
REPLACE_KEEPS_$0 || REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE
Copy link
Owner

Choose a reason for hiding this comment

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

A strange logic. Why ||?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that REPLACE_KEEPS_$0 is intended for lte IE11 but IE11 doesn't have the issue that REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE intends to resolve and vice-versa, so the conditional would end up false for both of them?

Copy link
Owner

Choose a reason for hiding this comment

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

It's not something related to specific engines, it's related to specific bugs and implementations of old spec versions. At this moment we have 3 replace-specific criteria - so for me seems logically to use && - REPLACE_SUPPORTS_NAMED_GROUPS && REPLACE_KEEPS_$0 && !REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattclough1 Note that the &&s are wrapped inside !(...), so the polyfill would be applied when at least one of the tests is false

@zloirock
Copy link
Owner

zloirock commented Jan 9, 2020

At this moment, fixRegExpWellKnownSymbolLogic is the worst part of core-js - spaghetti, it seems required to refactor it in the near future...

… for guard in fix-regexp-well-known-symbol-logic
(KEY === 'replace' && !(
REPLACE_SUPPORTS_NAMED_GROUPS &&
REPLACE_KEEPS_$0 &&
REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE
Copy link
Owner

Choose a reason for hiding this comment

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

Other criteria mean that the implementation is correct, but REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE that it's buggy, so seem it should be !REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE.

@zloirock
Copy link
Owner

I'll finish it by myself, thanks for the PR.

@zloirock zloirock merged commit 254a227 into zloirock:master Jan 10, 2020
zloirock added a commit that referenced this pull request Jan 10, 2020
@mattclough1 mattclough1 deleted the issue-471 branch January 10, 2020 14:49
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Oprava

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updates to String.prototype.replace doesn't handle $ in replacement value without capture group
3 participants