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

fix: remove() should work with multichar strings #165

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Collaborator

@Trott Trott commented Aug 18, 2022

Fixes: #164

@Trott
Copy link
Collaborator Author

Trott commented Aug 18, 2022

This preserves the current behavior but at the expense of a duck-type check for a regular expression combined with a new RegExp(options.remove, 'g'). I'd want to think hard about edge cases where this might be a security issue. Rather than do that thinking, I prefer the straightforward (but small breaking change) approach in #166.

@Trott
Copy link
Collaborator Author

Trott commented Aug 18, 2022

Another argument for the breaking change instead of this fix: This preserves the "replace all matches of a regular expression even if it doesn't have a 'g' flag" behavior...but only for regular expressions. Strings will now only replace the first instance. (This is still a bug fix for some strings, as previously the strings didn't work at all if there were multicharacter strings specified, as in the linked issue. Those now work, ableit only for the first match.)

@Trott Trott closed this Aug 18, 2022
@Trott Trott deleted the option1 branch August 18, 2022 23:58
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.

Remove function isn't working properly
1 participant