Remove function isn't working properly
Toast1 opened this issue · 7 comments
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.
This is indeed a bug in slugify. I'll push a fix soon (unless @simov does it first). If you want to use slug instead, this works:
slug(input, {remove: /AF/ig})Thank you so much for the quick response!
It surely is nice feature and otherwise Slugify works great!
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 slugify('AFdAFd', {remove: /[AF]+/g}) works as expected.
So maybe either release a new major or document that you have to use a character class in your remove regex? Assuming that most people are doing that anyway as that is the intended goal for the module.
Though note that
slugify('AFdAFd', {remove: /[AF]+/g})works as expected.
FWIW, the + is superfluous. (One might think the g is superfluous too, but there is the edge case where we add more than one character at a time.) slugify('AFdAFd', {remove: /[AF]/g}) works just as well with the current code. The problem arises when someone wants to remove AF but not AB or EF.
So maybe either release a new major or document that you have to use a character class in your
removeregex? Assuming that most people are doing that anyway as that is the intended goal for the module.
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:
- Strings are only reliable if they are a single character
- Regular expressions must be a character class
- Use of regular expressions that include things other than character classes are not supported and may have unexpected results.
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.