fixes #471 #745

mattclough1 · 2020-01-09T18:29:48Z

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?

zloirock · 2020-01-09T18:56:31Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

@@ -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']) {


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

zloirock · 2020-01-09T18:57:36Z

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

zloirock · 2020-01-09T21:33:46Z

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 · 2020-01-09T21:55:08Z

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!

zloirock · 2020-01-09T21:46:06Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

@@ -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')]) {


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

zloirock · 2020-01-09T21:46:35Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

+// 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') === '';


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

zloirock · 2020-01-09T21:52:04Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

@@ -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


A strange logic. Why ||?

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?

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.

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

zloirock · 2020-01-09T22:23:41Z

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

zloirock · 2020-01-10T06:45:53Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

+    (KEY === 'replace' && !(
+      REPLACE_SUPPORTS_NAMED_GROUPS &&
+      REPLACE_KEEPS_$0 &&
+      REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE


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 · 2020-01-10T12:22:26Z

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

ghost

Oprava

checks if RegExp[Symbol.replace] substitutes undefined capture groups

7948175

zloirock reviewed Jan 9, 2020

View reviewed changes

Matt Clough added 2 commits January 9, 2020 14:12

use wellKnownSymbol util to get symbol key for replace method

3bfe93c

fixes REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE conditional logic

e0a3d42

zloirock reviewed Jan 9, 2020

View reviewed changes

caches Symbol.replace as constant, uses regexp literal, swaps default…

c7429d1

… for guard in fix-regexp-well-known-symbol-logic

zloirock reviewed Jan 10, 2020

View reviewed changes

zloirock merged commit 254a227 into zloirock:master Jan 10, 2020

zloirock added a commit that referenced this pull request Jan 10, 2020

some missed actions for #745

6f80aac

mattclough1 deleted the issue-471 branch January 10, 2020 14:49

This was referenced Jan 19, 2020

[Snyk] Upgrade core-js from 3.6.2 to 3.6.3 VonSwirl/ES6-boilerplate-webapp#26

Merged

[Snyk] Upgrade core-js from 3.6.1 to 3.6.3 hagoraya/Todo-App#1

Open

[Snyk] Upgrade core-js from 3.2.1 to 3.6.3 secureCodeBox/securecodebox.github.io#7

Closed

snyk-bot mentioned this pull request Feb 20, 2020

[Snyk] Upgrade core-js from 3.4.5 to 3.6.4 Narshe1412/Code-Institute-Data-Centric-Project#153

Merged

snyk-bot mentioned this pull request Feb 29, 2020

[Snyk] Upgrade core-js from 3.6.3 to 3.6.4 VonSwirl/ES6-boilerplate-webapp#34

Merged

This was referenced Mar 13, 2020

[Snyk] Upgrade core-js from 3.5.0 to 3.6.4 svoboda-vlad/test-jhipster-upload-from-csv#11

Closed

[Snyk] Upgrade core-js from 3.5.0 to 3.6.4 CodeForBaltimore/Healthcare-Rollcall#43

Merged

[Snyk] Upgrade core-js from 3.4.8 to 3.6.4 ssmbct-netops/CyberChef#1

Open

vaibhavarora14 mentioned this pull request Apr 8, 2020

[Snyk] Upgrade core-js from 3.1.4 to 3.6.4 vaibhavarora14/Banks_List_Demo#14

Merged

snyk-bot mentioned this pull request Apr 25, 2020

[Snyk] Upgrade core-js from 3.0.1 to 3.6.5 rollup-umd/react-styleguidist#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade core-js from 3.6.0 to 3.6.5 MitchTalmadge/CometCrate#30

Merged

ghost reviewed Oct 3, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #471 #745

fixes #471 #745

mattclough1 commented Jan 9, 2020 •

edited

zloirock Jan 9, 2020

zloirock commented Jan 9, 2020

zloirock commented Jan 9, 2020

mattclough1 commented Jan 9, 2020 •

edited

zloirock Jan 9, 2020

zloirock Jan 9, 2020

zloirock Jan 9, 2020

mattclough1 Jan 9, 2020

zloirock Jan 9, 2020

nicolo-ribaudo Jan 9, 2020

zloirock commented Jan 9, 2020

zloirock Jan 10, 2020

zloirock commented Jan 10, 2020

ghost left a comment

fixes #471 #745

fixes #471 #745

Conversation

mattclough1 commented Jan 9, 2020 • edited

Choose a reason for hiding this comment

zloirock commented Jan 9, 2020

zloirock commented Jan 9, 2020

mattclough1 commented Jan 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zloirock commented Jan 9, 2020

Choose a reason for hiding this comment

zloirock commented Jan 10, 2020

ghost left a comment

Choose a reason for hiding this comment

mattclough1 commented Jan 9, 2020 •

edited

mattclough1 commented Jan 9, 2020 •

edited