Use require.resolve as a fallback of path.resolve

[kachkaev]

Hi @DavidAnson! We had a discussion about resolving config.extends in the CLI some time ago; this PR is a follow-up.

My initial question a few months ago was about extending js files in an auto-discovered json config and you explained some unwanted security implications of a change in the logic. We also discussed an opportunity of using require.resolve as a risk-free option for those who want to use a shared markdownlint config and avoid writing something like this in their .markdownlint.json:

{
  "extends": "./node_modules/@my-company/markdownlint-config/index.json",
  "first-line-heading": false,
  "no-inline-html": false
}

This is what used to stick with till now.

This weekend I was playing with Yarn v2 (Berry) and realised that the conversion of "./node_modules/@my-company/markdownlint-config/index.json" to "@my-company/markdownlint-config" became unavoidable. The folder called node_modules no longer exists and the real location of all dependency files is now fully controlled by Yarn. Thus, there is no way to refer to a config file in a third-party package without using require.resolve.

This PR adds require.resolve as a fallback to path.resolve in a fully backwards-compatible manner. It should not affect the security model and does not change the error messages in the existing scenarios. The resolution strategy just becomes a little bit more optimistic and can successfully handle more cases than it used to before. So the change in the logic remains unnoticeable for the existing Markdownlint users, while making it possible to use fancier and shorter "extends" paths.

I'd be keen to hear what you think about this change and if you're generally happy with the approach, I'm happy to add tests and resolve any other review comments you might have. WDYT?

[DavidAnson]

Since then, I have used require.resolve in CLI2, so I am more familiar with it. Thanks for bringing this up again! I will have a look at the changes. I'd definitely want to add test cases for the new scenarios. FYI that I'm preparing for a patch release of the library shortly, so would wait to merge this PR until that went out.

[DavidAnson]

Makes sense! I left some comments/questions. Will do another review after you have a chance to go over them. Thanks!

[kachkaev]

Thanks for a quick reply @DavidAnson! I'm happy to address your comments within 12 hours – just going to sleep now 🙂

[kachkaev]

Alternatively, feel free to tweak the PR yourself if that’s preferred. I don’t worry much about the authorship, so will be also happy is you close this PR and implement require.resolve fallback yourself – whatever you find easier. Speak soon!

[DavidAnson]

No hurry. Get some sleep!

[DavidAnson]

This is looking really good, thank you!

[DavidAnson]

Thanks again! Left a couple more minor comments. This is probably the last round since I am getting nit-picky. I may end up tweaking the changes very slightly after merging, but that's better than giving you a hard time.

[DavidAnson]

Please also update the PR to be against next branch instead of main.

[kachkaev]

Thanks for another review @DavidAnson! Happy to do address more comments if you want – no rush with the merge. Let’s do it properly 💪

[DavidAnson]

Looks great! I left one more tiny comment. I'll probably release the patch version in the next couple of days, then can merge this into next branch.

Thanks!!

[fisker]

I'm the author of unicorn/prefer-optional-catch-binding, why we enabled this rule, but used a lot // eslint-disable-next-line unicorn/prefer-optional-catch-binding?

[DavidAnson]

@fisker That rule is enabled because I agree with it and it looks to be supported by Node version 10+ (as this library is). You may know better, but I'm guessing that I have ESLint slightly misconfigured to be using a language version that does not allow the optional binding? I will look into that and fix it if so.

[fisker]

Ah, the ecmaVersion is misconfiged,

markdownlint/.eslintrc.json

Line 3 in 82cf680

"ecmaVersion": 2018

I think it should be at least 2019?

[DavidAnson]

@fisker Yes, probably. If there is a good mapping from Node version to this setting, I'd love to know! I will update this tomorrow and remove the unnecessary suppressions. Thanks!

[fisker]

This? https://node.green/#ES2019

[DavidAnson]

@fisker I like that site and it's what I use, but note that some of those features are supported by Node 10 and some are not. I guess what I really want is to specify the minimum Node version to ESLint rather than the JavaScript version. No big deal; I will go with 2019 and make the associated updates. Thanks again!

[kachkaev]

Thanks again for reviewing and merging this PR @DavidAnson! Looking forward to trying it out in the released version 🚀

[kachkaev]

👋 @DavidAnson! What are your thoughts on releasing the next version, which would include this change?

[DavidAnson]

I picked out 6 "enhancement" issues to include with the next release. I'm about half way through them now. Once done, I plan to publish!