-
Notifications
You must be signed in to change notification settings - Fork 88
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
Replace linkify-urls with linkifyjs #1678
Conversation
Linkify-urls is redundant as we already had a library. Linkify-urls is not compatible with Safari as it's designed to be used on Node JS and it uses non-polyfillable regexp features. Signed-off-by: Vincent Petry <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works
Is it a good idea to replace a 7kb dependency with a >500kb one? |
Linkify is already a dependency. |
so far, as this is mostly fire fighting, I'd like to move forward with this fix and not use two different libraries. separately we could look into finding a smaller library then and use only that one everywhere I think @dartcafe was referring to the old version of linkify-urls which doesn't seem to have the back-references in the regexp https://github.com/sindresorhus/linkify-urls/blob/v2.2.0/index.js#L5 vs https://github.com/sindresorhus/linkify-urls/blob/2d04fd8121e6adc33d35f1303495966a39f0dfa8/index.js#L5. |
I've raised #1679 for researching a smaller dep that works everywhere |
SO, what was the decision regarding #1488 ? |
follow up in #1488 there was no further discussion about it. my PR was just an emergency thing to unblock a release due to Safari regressions |
Linkify-urls is redundant as we already had a library.
Linkify-urls is not compatible with Safari as it's designed to be used
on Node JS and it uses non-polyfillable regexp features.
@nickvergessen please test this with Safari, also with Talk.