tools: eliminate intermediate module in doctools

[vsemozhetbyt]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Fixes: #20685

We already check this path in doctools tests, so why not to require this module directly without unneeded overhead:

node/test/doctool/test-doctool-html.js

Lines 4 to 9 in 6de0f55

	// The doctool currently uses js-yaml from the tool/node_modules/eslint/ tree.
	try {
	require('../../tools/node_modules/eslint/node_modules/js-yaml');
	} catch (e) {
	common.skip('missing js-yaml (eslint not present)');
	}

node/test/doctool/test-doctool-json.js

Lines 4 to 9 in 6de0f55

	// The doctool currently uses js-yaml from the tool/node_modules/eslint/ tree.
	try {
	require('../../tools/node_modules/eslint/node_modules/js-yaml');
	} catch (e) {
	common.skip('missing js-yaml (eslint not present)');
	}

It seems we can leave this block unchanged, as this package.json is more informational than functional:

node/tools/doc/package.json

Lines 12 to 14 in 6de0f55

	"devDependencies": {
	"js-yaml": "^3.5.2"
	},

[vsemozhetbyt]

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/700/

[Trott]

This depending-on-a-module-in-an-unrelated-source-tree seems like a code smell to me. Wouldn't it be better to simply remove the checks from the tests (after making sure that the code doesn't use the version in eslint someplace else)?

[Trott]

I'm opposed to fast-tracking because I don't think this is the right approach, at least based on the context I have. It's possible that I don't fully understand the situation.

[vsemozhetbyt]

TBH, I do not understand why we need tools/doc/package.json at all: doctools is not an outer installed package now, just a bunch of our internal scripts.

So I am not sure how to proceed. Doctools only require js-yaml in tools/doc/common.js, but use it in both MD-2-HTML and MD-2-JSON converters.

Can we have cases when we build docs or run doctools tests without ESLint in the tree?

[Trott]

Oh...I see. The js-yaml in doc/tools just loads the one from eslint anyway. /ping @addaleax who set it up that way partly (or entirely?) to avoid bloat in the repo.

I don't oppose this change. No objection from me. I do think we shouldn't fast-track it. Let's give @addaleax and anyone else who may have set things up this way a chance to look at it.

[addaleax]

Hi! 👋 I don’t have any strong feelings over this. It kind of trades one hack for another. :)

I think that ideally, what we’ll have at some point is just a global package.json and node_modules at the top level of our repository, for all npm dependencies that we have somewhere in the source tree. But I’m not sure how everybody else would feel about that. 😄

[vsemozhetbyt]

Landed in 01e2f48
Thank you for the reviews.