[PR #342] [MERGED] Use require.resolve as a fallback of path.resolve #842

New issue

Closed

opened 2026-03-03 01:30:25 +03:00 by kerem · 0 comments

kerem commented

2026-03-03 01:30:25 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/DavidAnson/markdownlint/pull/342
Author: @kachkaev
Created: 10/17/2020
Status: ✅ Merged
Merged: 10/22/2020
Merged by: @DavidAnson

Base: next ← Head: fallback-to-require-resolve

📝 Commits (10+)

c518e92 Use require.resolve as a fallback of path.resolve
80a76f5 Remove unnecessary catch block
d2800c8 Address some comments and add a couple of tests
b0c0091 Address some comments
1e0e339 Fix incorrect c8 report on branches
5d5a257 Tweak nodejs.yml to remove c8 ignore
8a66f80 resolved → Resolved
b08b5b4 Address some review comments
a0a069b Add paths to require.resolve
2854f3b Update .github/workflows/nodejs.yml

📊 Changes

9 files changed (+5672 additions, -8 deletions)

View changed files

📝 .github/workflows/nodejs.yml (+2 -2)
📝 .gitignore (+1 -0)
📝 lib/markdownlint.js (+32 -6)
➕ package-lock.json (+5590 -0)
➕ test/config/config-badchildpackage.json (+4 -0)
➕ test/config/config-packageparent.json (+7 -0)
📝 test/markdownlint-test.js (+29 -0)
➕ test/node_modules/pseudo-package/config-frompackage.json (+4 -0)
➕ test/node_modules/pseudo-package/package.json (+3 -0)

📄 Description

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?

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/DavidAnson/markdownlint/pull/342 **Author:** [@kachkaev](https://github.com/kachkaev) **Created:** 10/17/2020 **Status:** ✅ Merged **Merged:** 10/22/2020 **Merged by:** [@DavidAnson](https://github.com/DavidAnson) **Base:** `next` ← **Head:** `fallback-to-require-resolve` --- ### 📝 Commits (10+) - [`c518e92`](https://github.com/DavidAnson/markdownlint/commit/c518e92f7fce85e6101550d9c5dec60a4aaa4f6b) Use require.resolve as a fallback of path.resolve - [`80a76f5`](https://github.com/DavidAnson/markdownlint/commit/80a76f5e89e8bc502469ff5a57517f101d7c7a76) Remove unnecessary catch block - [`d2800c8`](https://github.com/DavidAnson/markdownlint/commit/d2800c8c0cac6239c5dc4096ae9697e2d84731dd) Address some comments and add a couple of tests - [`b0c0091`](https://github.com/DavidAnson/markdownlint/commit/b0c009113385cf3deeda5892a3ac2d8741b3b22e) Address some comments - [`1e0e339`](https://github.com/DavidAnson/markdownlint/commit/1e0e3396287d3c5044a2ce9b734a0a5cc92814a6) Fix incorrect c8 report on branches - [`5d5a257`](https://github.com/DavidAnson/markdownlint/commit/5d5a2572bdbc5db6272fabdd3f94d07a61f927f0) Tweak nodejs.yml to remove c8 ignore - [`8a66f80`](https://github.com/DavidAnson/markdownlint/commit/8a66f8082c689b6ed30d532c80c9c48b60752391) resolved → Resolved - [`b08b5b4`](https://github.com/DavidAnson/markdownlint/commit/b08b5b413f1fb8d26474a221aa1e40ac630f8990) Address some review comments - [`a0a069b`](https://github.com/DavidAnson/markdownlint/commit/a0a069bcee6370e9134d363de0b12862cfd3b86e) Add paths to require.resolve - [`2854f3b`](https://github.com/DavidAnson/markdownlint/commit/2854f3bf5b6a58e5a305479e331e9a4cfb2004dc) Update .github/workflows/nodejs.yml ### 📊 Changes **9 files changed** (+5672 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/nodejs.yml` (+2 -2) 📝 `.gitignore` (+1 -0) 📝 `lib/markdownlint.js` (+32 -6) ➕ `package-lock.json` (+5590 -0) ➕ `test/config/config-badchildpackage.json` (+4 -0) ➕ `test/config/config-packageparent.json` (+7 -0) 📝 `test/markdownlint-test.js` (+29 -0) ➕ `test/node_modules/pseudo-package/config-frompackage.json` (+4 -0) ➕ `test/node_modules/pseudo-package/package.json` (+3 -0) </details> ### 📄 Description Hi @DavidAnson! We had a [discussion](https://github.com/igorshubovych/markdownlint-cli/issues/97) 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`: ```js { "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](https://github.com/kachkaev/njt/pull/29) with [Yarn v2 (Berry)](https://github.com/yarnpkg/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? --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>