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

Add 'keepSlashes' option #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KseniaTetyorkina
Copy link

Alternative solution for #130 (These changes help solve this problem: #98)

@Trott
Copy link
Collaborator

Trott commented Aug 27, 2021

I'm not opposed to this, but I would want to be very thoughtful about adding options in general. I'm not sure this isn't a niche feature and I don't know if we want to add an option every time something like this comes up and be on the hook to maintain different option configurations. (What is the expected behavior if someone enables keepSlashes but explicitly removes slashes using remove?)

Should this be more generic so we don't have to later add options for keepColons and keepSpaces? (I'm making those up, obviously, but you get the idea.)

This seems like something an end user could do easily enough:

function slugifyWithSlashes(myString) {
 return myString.split('/').map((val) => slugify(val)).join('/')
}

Anyway, I could go either way on this pull request. What are your thoughts, @simov?

@simov
Copy link
Owner

simov commented Aug 28, 2021

@KseniaTetyorkina I appreciate your effort on this one, but I agree with @Trott that this isn't the best way to solve this, by essentially patching a default regular expression with an option. The whole point of having a regular expression is to provide flexibility to the user without having to add explicit option for every single character.

That being said the defaults probably needs to be changed in the next major as outlined in the original issue.

@Trott
Copy link
Collaborator

Trott commented Aug 28, 2021

That being said the defaults probably needs to be changed in the next major as outlined in the original issue.

I always considered a slug to be a URL element, not a path, so keeping slashes by default would be a bug in that regard.

To me, the canonical use case is "I have a title of an article and I want to convert that article title to a component in a URL."

So I might have a blog at www.example.com/my-awesome-blog. And if I post something today, it might end up in www.example.com/my-awesome-blog/2021/08/28/some-slug-value-here.

So if I have an article titled "Better A/B Testing", I want a slug like better-ab-testing or Better-AB-Testing or better-a-b-testing but most certainly not better-a/b-testing.

The use case in that other issue (of passing a whole path) seems unusual to me. (It's possible I'm wrong, and I'd welcome demonstration of that. But the fact that it's never come up until now suggests otherwise.)

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.

3 participants