Fix MD Hyperlink Regex for nested cases

[mananjadhav]

@tgolen will you please review this?

Updated markdown regex to avoid nested cases

Fixed Issues

$ Expensify/App

Tests

1. Test url replacements
2. Test markdown style link with various styles
3. Test critical markdown style links
4. Test markdown and url links with inconsistent starting and closing parens

QA

1. What does QA need to do to validate your changes?
2. What areas to they need to test for regressions?

[aldo-expensify]

I tested with the following examples:

[Text text] more text ([link [square brackets within] here](www.google.com))
[Text text] more text ([link (parenthesis within) here](www.google.com))
[Text text] more text [link here](www.google.com)
[Text text] more text ([link here](www.google.com))
[Text text] more text ([link here  ](www.google.com))
[Text text] more text (([link here](www.google.com)))
[Text text] more text [([link here](www.google.com))]
[Text text] more text ([link here](www.google.com))[Text text] more text ([link here](www.google.com))

All of them seem to have worked fine:

Ran the test locally, they work fine.

[mananjadhav]

Thanks, @aldo-expensify for running additional test cases! Appreciate your help. Can we merge this then? So that I can raise a PR for App with updated hash?

[aldo-expensify]

@tgolen do you want to do a quick review before I merge?

[mananjadhav]

@aldo-expensify I added your additional test cases to the code itself. This will ensure future changes don't break these.

[tgolen]

The code changes look good. I'm a little confused by the lint errors that are showing up. Would you please merge main into your branch to ensure you're on the latest version of our lint rules? It was recently updated. We recently had a change that increases the max-len rule and should make these warnings go away.

[mananjadhav]

@tgolen Yeah I did see many warnings and I did take the latest master branch for expensify-common. Were the lint rules updated for expensify-common too?

Previous PRs that are merged also show up the same warnings.

[tgolen]

Ah, good catch, I think expensify-common did not have its dependency updated cc @iwiznia

https://github.com/Expensify/expensify-common/blob/master/package.json#L36

Since these aren't introduced in your PR, I'm OK with merging this and we can update the dependency in another PR.

[mananjadhav]

Thanks. I'll update the PR for App and link the GH issue.

[iwiznia]

Ah, good catch, I think expensify-common did not have its dependency updated cc @iwiznia

hmmm so did I miss updating something and the change is not really applied in the projects?

[tgolen]

I'm not really sure how NPM resolves this, but there are competing versions of the linter in this repo. I think the real proper way to change it is to go to expensify-common and change the eslint config to be a peer dependency instead of a normal dependency. That way, it will rely on the dependency from App rather than relying on it's own dependency.

[iwiznia]

hmmmm but right now it's in dev dependencies... so if I move it to peer dependency, wouldn't that mean it will get installed in non dev?

[tgolen]

Not that I know of, no... as long as the peer (App in this case) has it listed in dev dependencies.

[iwiznia]

But App does not have expensify-common as dev dependency....
I think I am not understanding this at all 😄

[tgolen]

But App does not have expensify-common as dev dependency....

This is beside the point. All we need is to have App list eslint-expensify-config as a dev dependency (which it does already) and then in expensify-common move eslint-expensify-config from a dev dependency to a peer dependency.

I'm not fully up to speed with how peer dependencies work, but I have worked with @kidroca on this with Onyx. See this example: https://github.com/Expensify/react-native-onyx/blob/master/package.json#L45-L48 (and more context in the original discussion can be found here).

Onyx depends on React and React-Async-Storage. When those were listed as a normal dependency, and the version was not in sync with App, when you installed App, it would install and use two different versions of React. Not good, right? So, we modified Onyx to use a peer dependency instead, which means it will only use React from the host application.

That's the same thing we want to do here.

If that's still confusing, or if you have more questions, I would advise to do your own research on peer-dependency (and maybe we both learn more from it, because I could still be wrong). You could also bring this up in the open source channel on Slack and discuss it with Kidroca because he seems to have a good understanding of how this works (more than me for sure).

[kidroca]

Hey I think I can shine some light into this

1. dev dependencies can't be moved to peerDependencies

Technically they can but they shouldn't
If you use something to aid development and it's not really part of the end source you can't be demanding it from the parent or a peer project
eslint-expensify-config should be part of devDependencies for each project that wants this convention

2. the pipeline for expensify-common does not live in the context of Expensify/App

Even outside of CI, if I just download and open expensify-common and want to write some code/tests or stuff - if eslint-expensify-config is part of peer dependencies nothing will install it for me (well npm7 will but still...) - it won't get installed by npm install

[kidroca]

If you don't want to keep the updating the hash across projects, you can define an eslint config (or a base config) in expensify-common and then use it in other projects through expensify-common

E.g. create a folder eslint or eslintConfig in expensify-common. then define eslint configurations there
then in projects like App or Onyx

module.exports = {
    extends: 'expensify-common/eslint/base', //or es6, react etc...
}

Or it can be as simple as reusing the config in common

module.exports = {
    extends: 'expensify-common/eslintrc',
}

I know extends works with a path, but I'm not sure whether you won't need to do something like:

extends: path.resolve('./node_modules/expensify-common/eslintrc')

[iwiznia]

hmmmm 🤔 I think I agree with you that we should add eslint-config as dev dependency everywhere, so they can be independent and we don't have to update common to update the config for the other projects.
Will wait for @tgolen to chime in.

[kidroca]

A case for peer dependencies is the dependency to eslint in eslint-config-expensify

Even though eslint-config-expensify is a dev dependency to you, eslint is a main dep for the config
But you don't want to force people into using eslint 6 or some other versions, or having to update the versions in package.json for each new release of eslint
A config would typically work with a wide range of eslint packages

In fact that's how it's advised to write eslint-config packages

You should declare your dependency on ESLint in package.json using the peerDependencies field. The recommended way to declare a dependency for future proof compatibility is with the ">=" range syntax, using the lowest required ESLint version.
https://eslint.org/docs/developer-guide/shareable-configs#publishing-a-shareable-config

[kidroca]

hmmmm 🤔 I think I agree with you that we should add eslint-config as dev dependency everywhere

Actually yeah, if you put it in common other packages would still have to update their version of common to get the new config. They might as well just update eslint-config-expensify

[tgolen]

OK, I'd be down with that. Thanks for chiming in @kidroca!

Did any of this ever explain why this PR is showing eslint errors when it shouldn't? 😬

[iwiznia]

Ohhh, this PR? Yes, I know, because we never really update the package in this repo it's pointing to a super old version