Fix: error from executing url regex#559
Conversation
|
@allroundexperts Should I move methods Url.getURLObject and ValidationUtils.isValidWebsite from App to this repo? Or just add try/catch to handle them in the App repo? |
allroundexperts
left a comment
allroundexperts
left a comment
There was a problem hiding this comment.
@eh2077 Can you please add tests for this?
|
@allroundexperts Added test |
| */ | ||
| containsNonPairTag(textToCheck: string): boolean; | ||
| extractLinksInMarkdownComment(comment: string): string[]; | ||
| extractLinksInMarkdownComment(comment: string): string[] | undefined; |
There was a problem hiding this comment.
Instead of undefined, lets return [] as it was doing previously?
There was a problem hiding this comment.
@allroundexperts I tend to stick with return undefined when exception occurs for the following reason. This method currently is only used by method getRemovedMarkdownLinks which needs to call this method twice to do diff to get the removed links. My point here is to let method getRemovedMarkdownLinks return empty array [] if any of the two links array is undefined, which means don't remove links if any exception occurs. Does it make sense to you?
Let's say, there's a new comment with multiple links but one of them causes regex execution error, in this case, we return undefined to skip removing any links. If we return [] for the new comment, then method getRemovedMarkdownLinks will remove all links.
New comment
asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfisjksksjsjssskssjskskssksksksksskdkddkddkdksskskdkdkdksskskskdkdkdkdkekeekdkddenejeodxkdndekkdjddkeemdjxkdenendkdjddekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdi.cdjd
www.google.com
www.expensify.com
Old comment
www.google.com
www.expensify.com
allroundexperts
left a comment
allroundexperts
left a comment
There was a problem hiding this comment.
I think it will also be good to log the failure for tracking purposes!
|
Bump @eh2077 |
@allroundexperts Added log messages. |
joelbettner
left a comment
joelbettner
left a comment
There was a problem hiding this comment.
Only requesting a couple small changes to the error messages.
|
@allroundexperts I found an error when sending log to backend using Logger setting from this repo https://github.com/Expensify/expensify-common/blob/main/lib/Log.jsx It looks like that the App doesn't include the jQuery in the build. 
The logger from App repo seems working well https://github.com/Expensify/App/blob/main/src/libs/Log.js Any suggestions on how to fix this issue? cc @joelbettner |
|
@eh2077 But we're adding If this is something that can be fixed quickly, then lets try to address that within this PR. If not, then we can comment out the log statements and create a new issue for fixing this. @joelbettner Let us know your thoughts. |
|
@allroundexperts I dug the logging error stack and I feel it's a bit tricky. I also found that jQuery may not be able to use in native platform. So I think we can use |
|
All yours @joelbettner! |
3 participants
Fixed Issues
$ Expensify/App#21266
Tests
QA
Same as test