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

Remove unnecessary dollar signs replacement #132

Merged
merged 1 commit into from Jun 20, 2019
Merged

Conversation

@thomlom
Copy link
Contributor

thomlom commented Apr 21, 2019

If I'm not mistaken, the function form of String.prototype.replace does not apply the special replacement patterns such as $$, $&, etc. Thus, it makes the $-prefixed regex replacement unnecessary in this case.

MDN Source

The function form of String.prototype.replace does not apply the special replacement patterns such as $$, $&, etc. Thus, it makes the $-prefixed regex replacement unnecessary in this case.
@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 21, 2019

I’m not sure that MDN is correct here for all engines, including older IE, so I’ll want to test it. This was added in #43 when we didn’t use the function form.

Could you add a test that demonstrates the bug with the current implementation, that your PR fixes?

@thomlom

This comment has been minimized.

Copy link
Contributor Author

thomlom commented Apr 22, 2019

  • I'm not sure what test you want me to add since there is already one checking for $-prefixed replacement:

    it('handles dollar signs in the substitution value', function () {

  • Whether we remove this line or not, the result stays the same:

    return replace.call(options[argument], dollarRegex, dollarBillsYall);

    This PR just intends to improve the performance a little bit by removing this unnecessary replacement.

  • As for all engines (IE, etc), I just have tested it in Node environments 🙂

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 24, 2019

Gotcha, so to be clear, this isn't a bugfix, it's a performance optimization - there's no actual harm in leaving it as-is?

Given that, as soon as I have time to test it in all browsers, then we can land it, but there's no rush.

@thomlom

This comment has been minimized.

Copy link
Contributor Author

thomlom commented Apr 25, 2019

Yes: just performance optimization!

That works for me 🙂

@thomlom

This comment has been minimized.

Copy link
Contributor Author

thomlom commented Jun 20, 2019

Hey @ljharb, I was just wondering if you had the time to check if that PR works in all browsers 🙂

@ljharb
ljharb approved these changes Jun 20, 2019
@ljharb ljharb merged commit 6c4faa1 into airbnb:master Jun 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.