Rework linkify-urls-in-code #467
Conversation
fregante
changed the title
|
These changes can be tested in
Let me know how the tests look |
fregante
changed the title
fregante
changed the title
|
Could you use https://github.com/sindresorhus/linkify-issues and https://github.com/sindresorhus/linkify-urls while you're at it? |
fregante
force-pushed
the
linkify-fix
branch
2 times, most recently
from
d2ee07b to
7e7825d
Compare
|
@sindresorhus damn you, you could have done that 48 hours ago :D |
fregante
force-pushed
the
linkify-fix
branch
2 times, most recently
from
743867c to
835700a
Compare
|
I think It only detects a single URL:
|
I was actually going to do a PR adding those after your "build script" PR, but you're just too fast. (Not complaining 😛) |
|
What do we do about |
|
@bfred-it I've worked around it for now: sindresorhus/linkify-urls@aca71b8 |
|
Great! I'll fix this today. I also found another issue that has to do with applying this to HTML rather than its text nodes. e.g. |
Before: * Its code was spread between utils, content and its own file * Matching was run three times (.test, .match, .replace) * The linkifier was only half-DRYed Now: * The replacer can run on new elements, it won’t stop if it finds any previously-linkified elements * The replacer is tested on elements
|
It was appearing here but |
src/libs/domify.js
Outdated
| // Shortcut for html`text` instead of html(`text`) | ||
| if (html.raw) { | ||
| html = String.raw(...arguments); | ||
| } |
There was a problem hiding this comment.
I don't like using tagged templates just for the sake of it. In this case it gives no added benefit over a normal function call, right?
| }; | ||
| walker[Symbol.iterator] = () => ({next}); | ||
| return walker; | ||
| }; |
There was a problem hiding this comment.
This would be a useful npm package. Hint hint ;) Shouldn't block merging this PR though.
There was a problem hiding this comment.
I thought about t but I'm not good at testing browser packages 😅
There was a problem hiding this comment.
Yeah, it's annoying, but easier if JSDom supports the tree-walker API. See: https://github.com/sindresorhus/is-blob/blob/927899acea40ce7bd31e2b3c4352248eba3a0609/test.js
src/libs/linkify-urls-in-code.js
Outdated
| } | ||
|
|
||
| // Mark code block as touched | ||
| untouchedCode.forEach(el => el.classList.add(linkifiedURLClass)); |
There was a problem hiding this comment.
Use a for-of loop for consistency and performance.
src/libs/util.js
Outdated
| // Export as functions because of the g flag | ||
| // https://stackoverflow.com/questions/1520800/why-regexp-with-global-flag-in-javascript-give-wrong-results | ||
| export const getURLRegex = () => /(http(s)?(:\/\/))(www\.)?[a-zA-Z0-9-_.]+(\.[a-zA-Z0-9]{2,})([-a-zA-Z0-9:%_+.~#?&//=]*)/g; | ||
| export const getIssueRegex = () => /([a-zA-Z0-9-_.]+\/[a-zA-Z0-9-_.]+)?#[0-9]+/g; |
There was a problem hiding this comment.
Shouldn't these be removed now?
There was a problem hiding this comment.
I suppose so. The rebase conflicts were confusing
| return; | ||
| } | ||
| for (const textNode of getTextNodes(el)) { | ||
| if (textNode.textContent.length < 11) { // Shortest url: http://j.mp |
There was a problem hiding this comment.
Realistically we can even increase the number to skip already-short URLs like that.
Which is approximately 90% on a diff page.
Tested with:
if (textNode.textContent.length < 11) { // Shortest url: http://j.mp
console.count('skipped')
continue;
}
console.count('ok')
|
Good to go?
|
|
Yup. It's perfect now. |
2 participants
First commit was to fix #463: If a line contained the same URL twice, it would replace the first URL twice instead of touching the second one.
Then I extended the changes to reorganize the code and test more of it.
Closes #463
Closes #478