fix: add support for localhost urls in markdown#535
Conversation
melvin-bot
Bot
requested review from
neil-marcellini
and removed request for
a team
|
@Santhosh-Sellavel any chance you can test this in App? |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks @allroundexperts looking good, is there an easy way to test this with the App before merging?
| const LOOSE_URL_REGEX = `(${LOOSE_URL_WEBSITE_REGEX}${URL_PATH_REGEX}(?:${URL_PARAM_REGEX}|${URL_FRAGMENT_REGEX})*)`; | ||
|
|
There was a problem hiding this comment.
Can we include the LOOSE_URL_REGEX along with the exported URL_REGEX as OR from here?
There was a problem hiding this comment.
Fixed. But we'll still need to import the both as well because of this condition.
@mountiny I've been testing it by applying the changes directly in |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
The tests look good but I'm a bit confused about the regex. I think you should also include test steps in App.
|
|
||
| const URL_REGEX_WITH_REQUIRED_PROTOCOL = URL_REGEX.replace(`${URL_PROTOCOL_REGEX}?`, URL_PROTOCOL_REGEX); | ||
|
|
||
| const LOOSE_URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}([-\\w]+(\\.[-\\w]+)*)(?:\\:${ALLOWED_PORTS}|\\b|(?=_))`; |
There was a problem hiding this comment.
Can you explain this chunk of the regex please? How did you come up with it and what is it supposed to do?
([-\\w]+(\\.[-\\w]+)*)My understanding is that it should match the domain name, however it isn't explained that way on regex101. Maybe I'm missing something...
There was a problem hiding this comment.
@neil-marcellini You need to replace the \\ with \ when testing it on regex101. The mentioned regex matches any word character along with a hyphen followed by an option . and any word character (hyphens included) zero or more times. This is added to cover domain names on local machines. These could be: localhost, a.b.c etc.
There was a problem hiding this comment.
Ah got it thanks!
You need to replace the
\\with\when testing it onregex101
Can you explain why?
There was a problem hiding this comment.
I'm not entirely sure but I think it might be due to the fact that we're having the regex in string form in our code and then we're converting it using new Regex.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@allroundexperts could you show tests with the App please?
@mountiny Sure. 
|
| replacedText = replacedText.concat(textToCheck.substr(match.index, (match[0].length))); | ||
| } else { | ||
| const urlRegex = new RegExp(`^${URL_REGEX}$`, 'i'); | ||
| const urlRegex = new RegExp(`^${LOOSE_URL_REGEX}$|^${URL_REGEX}$`, 'i'); |
There was a problem hiding this comment.
Why can't we use MARKDOWN_URL_REGEX here?
There was a problem hiding this comment.
@Santhosh-Sellavel We can't use it because ^${LOOSE_URL_REGEX}$|^${URL_REGEX}$ is different from ^${LOOSE_URL_REGEX}|${URL_REGEX}$. We want it to EITHER start and end with loose url regex OR start and end with URL_REGEX.
allroundexperts
dismissed stale reviews from neil-marcellini and mountiny
via
13ea2b4
Santhosh-Sellavel
left a comment
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
Looks good to me, tests well. Will continue on App PR thanks!
|
bump @mountiny or @neil-marcellini |
| const URL_REGEX_WITH_REQUIRED_PROTOCOL = URL_REGEX.replace(`${URL_PROTOCOL_REGEX}?`, URL_PROTOCOL_REGEX); | ||
|
|
||
| const LOOSE_URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}([-\\w]+(\\.[-\\w]+)*)(?:\\:${ALLOWED_PORTS}|\\b|(?=_))`; | ||
| const LOOSE_URL_REGEX = `((${LOOSE_URL_WEBSITE_REGEX})${URL_PATH_REGEX}(?:${URL_PARAM_REGEX}|${URL_FRAGMENT_REGEX})*)`; |
There was a problem hiding this comment.
We allowed the URL to start by hyphen which is not allowed according to the RFC1034. This caused Expensify/App#27080
4 participants
This PR adds support for auto linking of local urls like
http://my-localhost,http://localhost:3000Fixed Issues
$ Expensify/App#17828
Tests
What unit/integration tests cover your change? What autoQA tests cover your change?
URL-test,ExpensiMark-HTML-testandExpensiMark-HTMLToText-testWhat tests did you perform that validates your changed worked?
Added new test cases that auto link local urls.
QA
What does QA need to do to validate your changes?
Run the new test cases
What areas to they need to test for regressions?
Check if urls are getting auto linked correctly