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

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

Closed
jmmorris opened this issue Jan 7, 2019 · 3 comments · Fixed by #745

Comments

@jmmorris
Copy link

jmmorris commented Jan 7, 2019

The recent updates to String.prototype.replace are causing issues when a replacement without a capture group starts with a $.

I was specifically seeing this in Safari 12 but didn't dig in on that front beyond knowing that it wasn't affecting Chrome.

Chrome: '{price} Retail'.replace(/{price}/g, '$25.00') == '$25.00 Retail'
Safari: '{price} Retail'.replace(/{price}/g, '$25.00') == '5.00 Retail'

Version: 2.6.1

@nicolo-ribaudo
Copy link
Contributor

I'll take a look

nicolo-ribaudo added a commit to nicolo-ribaudo/core-js that referenced this issue Jan 7, 2019
@nicolo-ribaudo nicolo-ribaudo mentioned this issue Jan 7, 2019
nicolo-ribaudo added a commit to nicolo-ribaudo/core-js that referenced this issue Jan 8, 2019
zloirock added a commit that referenced this issue Jan 9, 2019
@dan-kez
Copy link

dan-kez commented May 22, 2019

I believe this is an issue still. I've tested with the following:

var url = "https://cdnjs.cloudflare.com/ajax/libs/core-js/2.6.8/core.js";
function loadjs(file) {
            var script = document.createElement("script");
            script.type = "application/javascript";
            script.onload=function(){
                //at this tine the script is loaded
                console.log("Script loaded!");
				console.log(String.prototype.replace)
				console.log('{price} Retail'.replace(/{price}/g, '$25.00'))
            }
            script.src = file;
            document.body.appendChild(script);
        }

loadjs(url);

Safari (In the console this is confirmed to go to the right location:

[Log] Script loaded!
[Log] function replace(searchValue, replaceValue) {
      var O = defined(this);
      var fn = searchValue == undefined ? undefined : searchValue[REPLACE];
      return fn !== undefined
        ? fn.call(searchValue, O, replaceValue)
        : $replace.call(String(O), searchValue, replaceValue);
    }
[Log] 5.00 Retail

@webschik
Copy link

webschik commented Sep 4, 2019

Safari behaves differently when RegExp.prototype[@@replace] is used with a replacement text $0-9:

RegExp.prototype[Symbol.replace].call(/{price}/g, '{price} Retail', '$25.00') // Safari - 5.00 Retail
RegExp.prototype[Symbol.replace].call(/{price}/g, '{price} Retail', '$25.00') // FF, Chrome - $25.00  Retail

RegExp.prototype[Symbol.replace].call(/{price}/g, '{price} Retail', '$test') // Safari, FF, Chrome - test Retail

And as I see RegExp.prototype[@@replace] is used in String.prototype.replace polyfill - https://cdnjs.cloudflare.com/ajax/libs/core-js/2.6.8/core.js:1862

It can be fixed by escaping $ with $$ - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Specifying_a_string_as_a_parameter

@mattclough1 mattclough1 mentioned this issue Jan 9, 2020
zloirock pushed a commit that referenced this issue Jan 10, 2020
* checks if RegExp[Symbol.replace] substitutes undefined capture groups

* use wellKnownSymbol util to get symbol key for replace method

* fixes REGEXP_REPLACE_SUBSTITUTES_UNDEFINED_CAPTURE conditional logic

* caches Symbol.replace as constant, uses regexp literal, swaps default for guard in fix-regexp-well-known-symbol-logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants