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

Replace linkify-urls with linkifyjs #1678

Merged
merged 1 commit into from
Jan 25, 2021
Merged

Conversation

PVince81
Copy link
Contributor

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.

image

@nickvergessen please test this with Safari, also with Talk.

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 <vincent@nextcloud.com>
@PVince81 PVince81 added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature labels Jan 22, 2021
@PVince81 PVince81 self-assigned this Jan 22, 2021
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works

@dartcafe
Copy link
Contributor

Is it a good idea to replace a 7kb dependency with a >500kb one?
Just mentioning: #1488
And linkify-urls@2 works great, also in Safari.

@nickvergessen
Copy link
Contributor

Linkify is already a dependency.
And at least the current linkify-urls causes a parse error on safari, which is why Talk and the vue lib docs are not working on recent safari

@PVince81
Copy link
Contributor Author

PVince81 commented Jan 25, 2021

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.
But I'm not sure if we'll want to use an outdated / old major of a library. Maybe there's yet another library that would fit.
This will need further research.

@PVince81 PVince81 merged commit d57e6d4 into master Jan 25, 2021
@PVince81 PVince81 deleted the bugfix/noid/switch-to-linkifyjs branch January 25, 2021 07:24
@PVince81
Copy link
Contributor Author

I've raised #1679 for researching a smaller dep that works everywhere

@skjnldsv
Copy link
Contributor

SO, what was the decision regarding #1488 ?
Because linkifyjs uses jQuery and is quite heavy for a simple string replacement.

@PVince81
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants