dodo / node-slug

slugifies even utf-8 chars!
MIT License
1.08k stars 92 forks source link

Not properly slugifying #16

Closed niftylettuce closed 9 years ago

niftylettuce commented 10 years ago

"It's your journey ... we guide you through."

converts to

It\'s-your-journey-...-we-guide-you-through.

which is invalid

dodo commented 10 years ago

what's the problem? that there is a \ in the string or the ...? for the first: did you just typed slug("It's your journey ... we guide you through.") in the node repl? when yes, try:

console.log(slug("It's your journey ... we guide you through."))

for the second: try this:

console.log(slug("It's your journey ... we guide you through.".replace(/[.]/g, ''))

or do you disagree with having ... in an url?

niftylettuce commented 10 years ago

disagree with having ... in a url, disagree with having apostrophies in a URL, should be /A-Z/i, /0-9/i, hyphen, and unicode characters only.

On Wed, Feb 19, 2014 at 4:50 PM, ▟ ▖▟ ▖ notifications@github.com wrote:

what's the problem? that there is a \ in the string or the ...? for the first: did you just typed slug("It's your journey ... we guide you through.") in the node repl? when yes, try:

console.log(slug("It's your journey ... we guide you through."))

for the second: try this:

console.log(slug("It's your journey ... we guide you through.".replace(/[.]/g, ''))

or do you disagree with having ... in an url?

— Reply to this email directly or view it on GitHubhttps://github.com/dodo/node-slug/issues/16#issuecomment-35553499 .

dodo commented 10 years ago

so, … you want a flavoured slug?

mariusa commented 10 years ago

I also encountered an issue with having "." in the URL. Would it be possible to replace "." too, please ? (e.g. with "-")

niftylettuce commented 10 years ago

can this be resolved/closed?

dodo commented 10 years ago

@niftylettuce i hope this is what you want.

niftylettuce commented 10 years ago

@dodo thanks for adding this option remove regex – but don't you think it'd be better to make this default? do you see any use case where a slug would have a period?

To browsers (and developers IMO), a period indicates an extension of a file type, and I know a lot of web app developers have route params that detect periods as an extension (e.g. ExpressJS's route params).

mariusa commented 10 years ago

+1 for excluding periods by default

BrunoBernardino commented 10 years ago

+1 for @mariusa and @niftylettuce 's request.

dodo commented 10 years ago

Thanks to @LinusU node-slug is respecting RFC 3986 which doesn't state that periods are bad. Expressjs's (dumb) approach to detect them as file extension is not my issue.

Anyway this is just my opinion and if you find a good reason i'll change the defaults. But for now, you can make it your own default:

slug.defaults.remove = /[.]/g
niftylettuce commented 10 years ago

I think this should be default. This package is for slugifying as opposed to conforming just to the RFC spec. I think you have to take into account best practice and also consider SEO/common usage. I know it's not in the spec but a majority use case for periods is file extensions as opposed to being used as part of a slug...

niftylettuce commented 10 years ago

Also the WordPress slug replaces dots with dashes – which is widely used (most popular CMS) – thanks to @yanicklandry for reminding me

niftylettuce commented 10 years ago

Some more backup... from Moz.com – Periods are not ok from the point of view of readability and accessiblity, which Google loves.

http://moz.com/community/q/is-using-dots-in-url-path-really-a-problem

dodo commented 10 years ago

how about we keep node-slug as generic as possible and you (or somebody else) create a module named seo-slug or something with all the sane defaults you propose. The thing is, that i would like to have a reason to change these defaults, not a tradition. If the reason is just readability ... why should i force people to get readable urls? ... maybe, the user chooses to not have the url readable. it all depends on the case and therefor removing periods by default doesn't belong in this library but into the lib/app/service where this lib is used.

offtopic IMHO if you're guessing the file type by its extension in the name, you're doing it wrong. (in http world you have mimetypes all over the place and in unix world you can test magic numbers and other stuff).

niftylettuce commented 10 years ago

Aren't slugs directly used to be for SEO purposes? I'd hate to clutter NPM with another package because one line of code that I'm sure 90% of people would want as a default. I guess I could fork... But I don't want to use GitHub as an NPM repo. Just trying once more. Bounty awarded if you decide to accept :) On Sep 24, 2014 7:08 PM, "▟ ▖▟ ▖" notifications@github.com wrote:

how about we keep node-slug as generic as possible and you (or somebody else) create a module named seo-slug or something with all the sane defaults you propose. The thing is, that i would like to have a reason to change these defaults, not a tradition. If the reason is just readability ... why should i force people to get readable urls? ... maybe, the user chooses to not have the url readable. it all depends on the case and therefor removing periods by default doesn't belong in this library but into the lib/app/service where this lib is used.

offtopic IMHO if you're guessing the file type by its extension in the name, you're doing it wrong. (in http world you have mimetypes all over the place and in unix world you can test magic numbers and other stuff).

— Reply to this email directly or view it on GitHub https://github.com/dodo/node-slug/issues/16#issuecomment-56752912.

niftylettuce commented 10 years ago

Not to mention a simple "slug" search on NPM yields a ton of results already, most of which I've already tried and/or contributed to. Your package is the very best AFAIK and this would make it utterly top notch. On Sep 24, 2014 8:32 PM, "Nick Baugh" niftylettuce@gmail.com wrote:

Aren't slugs directly used to be for SEO purposes? I'd hate to clutter NPM with another package because one line of code that I'm sure 90% of people would want as a default. I guess I could fork... But I don't want to use GitHub as an NPM repo. Just trying once more. Bounty awarded if you decide to accept :) On Sep 24, 2014 7:08 PM, "▟ ▖▟ ▖" notifications@github.com wrote:

how about we keep node-slug as generic as possible and you (or somebody else) create a module named seo-slug or something with all the sane defaults you propose. The thing is, that i would like to have a reason to change these defaults, not a tradition. If the reason is just readability ... why should i force people to get readable urls? ... maybe, the user chooses to not have the url readable. it all depends on the case and therefor removing periods by default doesn't belong in this library but into the lib/app/service where this lib is used.

offtopic IMHO if you're guessing the file type by its extension in the name, you're doing it wrong. (in http world you have mimetypes all over the place and in unix world you can test magic numbers and other stuff).

— Reply to this email directly or view it on GitHub https://github.com/dodo/node-slug/issues/16#issuecomment-56752912.

dodo commented 10 years ago

I think i nailed it:

require('slug/seo')

it's not pretty, but should work for now. feel free to change it as you like, send a PR and i'll update npm for you :)

dodo commented 10 years ago

Aren't slugs directly used to be for SEO purposes?

slugs generate a text representation that doesn't need a uri encoding any more.

niftylettuce commented 10 years ago

I don't like the slug/seo approach, I don't see what the argument is for having periods. Who will even use periods as part of a slug? It's not user-friendly nor helpful for accessibility. Slugs are mainly used for SEO as far as I'm concerned. If you didn't use it for SEO or user-friendliness, then you would just use variables like id directly in a URL. Ignoring the user, you can have routes like /news/2379182798127398127398217398 and not use a slug package. If you really won't make this default, I'll just fork and submit a new package and clutter NPM as I'd hope I'd not have to do. Please reconsider, thank you.

niftylettuce commented 10 years ago

If you had an apple tree, and the only way people could pick apples was to first set up a ladder to climb the tree – wouldn't you by default have a ladder set up for people to climb the tree to easily get an apple?

ndimatteo commented 9 years ago

+1 I'm all for having periods replaced or removed like WordPress does.

LinusU commented 9 years ago

I don't like the slug/seo approach. I do like having periods replaced by default.

I cannot think of a use case for not having periods replaced, but if there is one, how about a new mode parameter to the options object. { mode: 'rfc3986' } and { mode: 'pretty' }. pretty being the default one.

niftylettuce commented 9 years ago

The mode idea is cool. On Sep 27, 2014 11:28 AM, "Linus Unnebäck" notifications@github.com wrote:

I don't like the slug/seo approach. I do like having periods replaced by default.

I cannot think of a use case for not having periods replaced, but if there is one, how about a new mode parameter to the options object. { mode: 'rfc3986' } and { mode: 'pretty' }. pretty being the default one.

— Reply to this email directly or view it on GitHub https://github.com/dodo/node-slug/issues/16#issuecomment-57056047.

dodo commented 9 years ago

done. Thanks @LinusU for this great suggestion!

niftylettuce commented 9 years ago

:boom:

skerit commented 9 years ago

Yeah, any special character has no place in a slug. Compliant or not, it just doesn't belong there.

It's also annoying: apostrophes were being sent to our server as %27, but in the DB it was just ', so an extra step of decoding the string had to be added...

aurelien-defossez commented 8 years ago

I just came around this issue, and even though I understand why the period has to be treated, I don't understand why it has to be removed and not replaced by a '-' like the other characters?

So for instance, when I want to slug a login which has periods, they are simply removed and my resulting login looks strange: e.g. monica.bellucci -> monicabellucci. I'd prefer monica-bellucci.