tools: restore change of doc building function signatures to opts hashes

[jmm]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

tools

Description of change

/cc @thealphanerd In #3888 signatures of some functions used to build the docs were converted to take options hashes and then those changes were interpreted as the intrinsic cause of a test failure and reverted in #6680. The test did fail because the callsite signatures didn't match the updates, but passes once that's resolved. This PR restores the changes converting those signatures to take options hashes, and updates the callsites in the recently added test/doctool/test-doctool-html.js. See discussion in #6680 for more info.

make test passes for me locally, and anecdotal testing of NODE=node make doc-only (what #3888 enabled) works as expected.

Node version defaults

I retained the additions from #6680 that set a default node version:

nodeVersion = nodeVersion || process.version;

However, I'd note that those additions aren't covered by tests and that of the 3 of them, I'd consider 2 of them to be redundant with the one in tools/doc/html.js:toHTML.

ES6

I noticed that since I started #3888 some ES6 (template literals) were added to tools/doc/html.js. Some of the changes I made could be written considerably more elegantly using some ES6 (a little bit with object literal shorthand properties, and most of all with destructuring), but the purpose of #3888 was to make it possible to build the docs (which relies on the files in this PR) using an existing Node install, which may be earlier than the Node version for which the docs are being built. So it'd be worth keeping that in mind and perhaps limiting the features used in the doc build tools to those that'll work a few versions back. (Example: I think template literals and object literal shorthand properties are both available without a flag in v4, but not destructuring.)

[MylesBorins]

ci: https://ci.nodejs.org/job/node-test-pull-request/2587/

[jmm]

Looks like the CI failure is in sequential/test-next-tick-error-spin.js in one build and unrelated to this PR?

[addaleax]

I think these can be const

[jmm]

Thanks for the feedback @addaleax. Yes, they could be. FWIW when I first PRed those changes there was no const in that file and after the changes were first merged in #3888 and reverted in #6680 I didn't want to rock the boat with anything other than changes that had already been previously +1ed (since the original PR had a hard enough time landing). Also FWIW there's a bunch of other var in the file that could be const. The specific vars you commented on are each only referenced once, so honestly probably the best thing is to just eliminate those bindings and use opts where those are referenced. The same probably goes for at least some of the vars in render().

Let me know if you want me to make a change either way.

[addaleax]

@jmm Yes, the CI failure is unrelated. This changes only the function signatures, not the behaviour in any way, right?

[jmm]

@addaleax Ok, thanks for confirming. That is correct. Originally these changes were PR'ed along with a behavior change (accepting a --node-version opt instead of using process.version) in #3888, but this PR is not intended to change any behavior, only the signatures and the call sites.

[addaleax]

@jmm A good rule of thumb is that var -> const replacements can take place in those lines of a PR where the code’s being touched anyway so it doesn’t make running git blame & co harder.

I’d actually like it if you could make these changes (or remote the var = … as you suggested in favour of accessing the parameters object directly), but this LGTM either way. And don’t worry, I’d like to land it before #6943 so I can make sure there are no conflicts (or the burden of resolving them doesn’t fall back onto you).

[eljefedelrodeodeljefe]

LGTM. If no objections, we could go ahead merging this any way. I am suggesting pushing around code a lot in #6974 and can amend any nits there if someone likes me to.

[addaleax]

Sounds good, too. @eljefedelrodeodeljefe You wanna do the merge? ;)

[eljefedelrodeodeljefe]

Thanks. Landed in c1ffb9f

[jmm]

@addaleax

A good rule of thumb is that var -> const replacements can take place in those lines of a PR where the code’s being touched anyway so it doesn’t make running git blame & co harder.

Ok, thanks. I mentioned the others in case it'd be preferred to update them all in one go, but that makes sense, I'll try to keep that in mind.

I’d actually like it if you could make these changes (or remote the var = … as you suggested in favour of accessing the parameters object directly), but this LGTM either way. And don’t worry, I’d like to land it before #6943 so I can make sure there are no conflicts (or the burden of resolving them doesn’t fall back onto you).

If it were up to me I'd probably eliminate some (maybe all) of those bindings at this point -- and perhaps I should've done it that way in the first place. If someone wants me to update anything there, ping me, but it sounds like others are planning to do it along with other changes. Either way is fine by me.

Thanks everyone!

[addaleax]

@jmm I guess the rest can be done, as @eljefedelrodeodeljefe said above, in #6974 if that one lands, or basically whenever the next chance for that occurs. Feel free to weigh in over there!

[jmm]

Thanks @addaleax. I see that discussion has moved to #6999 -- I'll try to check that out sometime. (I just skimmed it real quick and saw your note about keeping it v4 compatible for the doc-only target -- thanks!) Not sure where that leaves the var|const|opts issue, but if someone wants me to make a change WRT that feel free to ping me.

[addaleax]

@jmm We will! In the meantime, there’s not much “discussion” going on in #6999 yet, but if you have an opinion on the path forward, you should feel free to share that.

[MylesBorins]

should this be backported?

[MylesBorins]

ping @nodejs/documentation