-
Notifications
You must be signed in to change notification settings - Fork 125
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 function isn't working properly #164
Comments
This is indeed a bug in slug(input, {remove: /AF/ig}) |
Thank you so much for the quick response! |
Fixes: simov#164 BREAKING CHANGE: remove() behavior now aligns with Stirng.prototype.replace(). To remove more than the first instance/match, use a regular expression with the global (g) flag.
I've coded up two fixes for this issue. One is in #165 and the other is in #166. I prefer the fix in #166 but that would be a breaking change and we'd want to do a major version bump for it. I still prefer it to the fix in #165, which still has at least one edge case lurking and adds a "pass user input to a constructor" call that I'd prefer to avoid lest there be hidden security implications or some other weirdness. @simov Do you have a preference? |
A major version bump would be an opportunity to drop support for older versions of Node.js and modernize the code base a little bit (but we don't have to do that--we can keep supporting Node.js 8 or whatever if we want to--it's just an option). |
I do prefer the #166 fix. This is one of those things that I inherited from the original code base. I think at some point someone tried fixing it, but in the end it was not published because of the breaking change. Though note that So maybe either release a new major or document that you have to use a character class in your |
FWIW, the
Oh, I hadn't considered that and to be honest, I don't know why. That's probably the best option. Just document the current quirks, which as far as I know are:
For completeness: Another possibility is that we could apply the regexp to the entire string every time and not just to the new characters being added at each step. That would seem to have both performance risks and the risk of unexpected results, though, and just increases complexity. I like "just document the quirks/limitations and leave it at that". I'm going to close the other pull requests, but we can re-open them if those solutions are preferred. I'll add a doc pull request later to update README.md with the bullet points above if no one else does and that can close this issue. |
Thank you for working on this @Trott 👍 |
Hello! I have a problem with custom remove function.
If I put only one custom character to be removed, it works fine and removes it in the whole string. But when there's more than one character, it doesn't remove anything. I also tried putting in custom RegEx, but it still didn't do anything.
Let's say my input is "AFdAFd"
Desired output would be "dd"
But real output is "afdafd"
I tried putting "AF" in to remove: . Also in RegEx form ( /(?:AF)+/g ), but nothing seems to work.
On the site it says you can remove characters, but after many attempts, I can't seem to achieve that.
The text was updated successfully, but these errors were encountered: