search

gregjacobs / Autolinker.js

Utility to Automatically Link URLs, Email Addresses, Phone Numbers, Twitter handles, and Hashtags in a given block of text/HTML
MIT License
1.48k stars 238 forks source link

Support scheme url starting with emoji #407

Closed caiofct closed 5 months ago

caiofct commented 5 months ago

This PR is a fix for https://github.com/gregjacobs/Autolinker.js/issues/399. It essentially allows a schema url to start with any char including an emoji.

Let me know if I've missed anything here since that's my first PR.

gregjacobs commented 5 months ago

Hey @caiofct, great stuff! Let me take a closer look when I get home tonight and we should be able to get it merged 👍

gregjacobs commented 5 months ago

Hey, so your first commit was good. The following 3 commits: not so much :)

image

When you're submitting a PR, you generally don't want to change a project's setup, tools, etc., unless you've first discussed it with the project's author. So for instance, changing a project from using npm to using yarn is a nono. Just because you like yarn better doesn't mean that everyone likes yarn better :) (p.s. yarn used to be good, but then they did some weird stuff and it's really annoying to install now, especially behind a corporate network. Sticking with npm at this point is fine, or use pnpm which is even better. But for a small open-source library, everyone already has npm if they have Node so not forcing contributors to figure out how to install new tools is ideal.)

Same with removing husky and puppeteer. Husky is used to run prettier on the Git precommit hook to keep code formatting consistent. And puppeteer is used in some of the integration tests that interact with a browser, so its removal breaks those tests.

And finally, the dist/commonjs and dist/es2015 folders were .gitignore'd for a reason. They're not needed for npm publishing, and would otherwise just cause many conflicts when merging concurrent PRs.

Bottom line: remove the last 3 commits and we should be able to merge :)

caiofct commented 5 months ago

Hey, so your first commit was good. The following 3 commits: not so much :)

image

When you're submitting a PR, you generally don't want to change a project's setup, tools, etc., unless you've first discussed it with the project's author. So for instance, changing a project from using npm to using yarn is a nono. Just because you like yarn better doesn't mean that everyone likes yarn better :) (p.s. yarn used to be good, but then they did some weird stuff and it's really annoying to install now, especially behind a corporate network. Sticking with npm at this point is fine, or use pnpm which is even better. But for a small open-source library, everyone already has npm if they have Node so not forcing contributors to figure out how to install new tools is ideal.)

Same with removing husky and puppeteer. Husky is used to run prettier on the Git precommit hook to keep code formatting consistent. And puppeteer is used in some of the integration tests that interact with a browser, so its removal breaks those tests.

And finally, the dist/commonjs and dist/es2015 folders were .gitignore'd for a reason. They're not needed for npm publishing, and would otherwise just cause many conflicts when merging concurrent PRs.

Bottom line: remove the last 3 commits and we should be able to merge :)

Sorry. I was doing some test locally and ended up also submitting it to this PR as well. I'll prepare another branch with only the first commit so we should be good to go.