Allow dashes according to RFC in all domain parts except TLD

markdown-it / linkify-it

Links recognition library with full unicode support

http://markdown-it.github.io/linkify-it/

MIT License

655 stars 63 forks source link

Allow dashes according to RFC in all domain parts except TLD #74

Closed Jikstra closed 5 years ago

Jikstra commented 5 years ago

Fixes https://github.com/markdown-it/linkify-it/issues/63

puzrin commented 5 years ago

https://github.com/markdown-it/linkify-it/issues/63#issuecomment-393289287 according to discussion, no options needed for correct implementation.
Test patterns should be placed in https://github.com/markdown-it/linkify-it/masterf/test/fixtures/links.txt when possible. Also, the real domain names required, not artificial ones.

Jikstra commented 5 years ago

63 (comment) according to discussion, no options needed for correct implementation.

So you want this to be the default behaviour? I'm not too familiar with markdown and it's limitiations, so i don't know when a url with multiple dashes would cause trouble.

Test patterns should be placed in https://github.com/markdown-it/linkify-it/masterf/test/fixtures/links.txt when possible. Also, the real domain names required, not artificial ones.

Thanks, will try to update the code accordingly.

Jikstra commented 5 years ago

@puzrin on more question, is http://a.b--c.de/ %--disabled, because collision possible still a url which shouldn't be allowed? Because writing a regex which checks that the dashes are only in the subdomain part is maybe a lot harder than just removing the small regex part which i made optional with the opts flag.

puzrin commented 5 years ago

So you want this to be the default behaviour?

Yes.

I'm not too familiar with markdown and it's limitiations, so i don't know when a url with multiple dashes would cause trouble.

If you do exactly as discussed in #63, that will be ok. We already discussed the best possible behaviour. Any deviation will require to do all checks again.

puzrin commented 5 years ago

New relaxed rule should touch only domain names (except TLD), and should not touch path.

Jikstra commented 5 years ago

My fix also allows urls like https://net--lify.com which you can't register (so i guess they are invalid). There was the one not-link test which i removed, i don't think this is a problem (and if so only a collission with markdown). If this is really a problem, i'm going to try to implement a regex which only allows multiple dashes in the second or lower levels.

Jikstra commented 5 years ago

hey @puzrin is anything still blocking you from merging this?

xPaw commented 5 years ago

Thanks @Jikstra!

Hopefully a package update can be pushed to npm too.

Jikstra commented 5 years ago

@xPaw no problem, the-lounge seems nice, know some people who are using it. I'm glad i could helped you :)