doc: add dynamic source code links

[aw-davidson]

Checklist

• commit message follows commit guidelines

[Trott]

Welcome, @aw-davidson, and thanks for the pull request.

What's the reason for wanting this? I see at least one problem with this, but it may be outweighed by a benefit if this solves problems for people.

The problem I see is that when these are rendered to HTML docs, it will mean that documentation for a specific version of Node.js will always link to the current master branch which can have very big differences.. I don't think that's what we want, but I don't know that there's an easy better solution either.

[aw-davidson]

@Trott, I thought it would be nice to add some visibility to the source code. Documentation for python and go have similar links to source code. I do see the issue with linking to master branch. Maybe something dynamic can be done here? https://github.com/nodejs/node/blob/master/tools/doc/html.js

[aw-davidson]

What if there was a comment in the markdown like that is replaced with an inline link at build time. And the inline link would link to a version branch. For example: https://github.com/nodejs/node/blob/v14.1.0/lib/buffer.js

[Trott]

What if there was a comment in the markdown like that is replaced with an inline link at build time. And the inline link would link to a version branch. For example: https://github.com/nodejs/node/blob/v14.1.0/lib/buffer.js

If you're up for implementing something like that, I think that would be great.

[aw-davidson]

@Trott, I made changes to dynamically link to the right version of the source code. You can now link to code with a comment in the markdown: <!-- source_link=lib/assert.js -->. I only added this for the assert module, but I would like to add it to all the other modules as well once I get feedback on these changes. Below is a picture with my changes.

[richardlau]

Can you undo the mode change for this file?

[aw-davidson]

Yeah, did not mean to commit that. I think I just changed it back.

[Trott]

@nodejs/documentation @nodejs/website

[aw-davidson]

I added links for all appropriate modules. Is it possible to retrigger the job that failed? I don't believe the failures would be related to my changes.

[richardlau]

I added links for all appropriate modules. Is it possible to retrigger the job that failed? I don't believe the failures would be related to my changes.

Failure from https://github.com/nodejs/node/pull/33996/checks?check_run_id=804738177 is

not ok 126 doctool/test-doctool-html
  ---
  duration_ms: 0.503
  severity: fail
  exitcode: 1
  stack: |-
    d:\a\node\node\test\common\index.js:602
    const crashOnUnhandledRejection = (err) => { throw err; };
                                                 ^
    
    TypeError: Cannot destructure property 'nodeVersion' of 'undefined' as it is undefined.
        at Function.preprocessText (d:\a\node\node\tools\doc\html.js:107:27)
        at freeze (d:\a\node\node\tools\doc\node_modules\unified\index.js:119:28)
        at Function.processSync (d:\a\node\node\tools\doc\node_modules\unified\index.js:384:5)
        at toHTML (d:\a\node\node\test\doctool\test-doctool-html.js:50:6)
        at d:\a\node\node\test\doctool\test-doctool-html.js:149:20
        at d:\a\node\node\test\common\index.js:365:15
        at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
  ...

I suspect the test will need to be updated.

[aw-davidson]

I suspect the test will need to be updated.

@richardlau, thanks for pointing that out. I just updated the one test that was failing and it looks like all the checks are good now.

[nodejs-github-bot]

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

[Trott]

I have some small changes I'd prefer, but rather than nit pick on this, I'll say "LGTM" and plan to do some modifications myself later (or not--they're not necessary, just my preference).

[Trott]

This will result in broken links when built on the master branch, but I'm not sure that's really a problem. But if there's an easy fix, let's fix it....

[Trott]

This will result in broken links when built on the master branch, but I'm not sure that's really a problem. But if there's an easy fix, let's fix it....

For example, I just built it on master and one of the links is https://github.com/nodejs/node/blob/v15.0.0/lib/assert.js. That will exist some day but...not right now.

[Trott]

Maybe someone from @nodejs/releasers can indicate if they think this is A+ compatible with the release process or not. (I think it should work fine. Looping in releasers just in case there's something interesting about when/how the version vX.XX tags are set and how it relates to nodeVersion/__VERSION__ in the doc templates.)

[jasnell]

Landed in e68563e ... was in the process of landing when you added your additional comments there @Trott, so if there's a reason this may need to have more work done or reverted just let me know.

[richardlau]

Maybe someone from @nodejs/releasers can indicate if they think this is A+ compatible with the release process or not. (I think it should work fine. Looping in releasers just in case there's something interesting about when/how the version vX.XX tags are set and how it relates to nodeVersion/__VERSION__ in the doc templates.)

nodeVersion is set as a passed in option to the doctool:

node/Makefile

Lines 747 to 749 in 122038c

	gen-api = tools/doc/generate.js --node-version=$(FULLVERSION) \
	--apilinks=$(LINK_DATA) $< --output-directory=out/doc/api \
	--versions-file=$(VERSIONS_DATA)

node/tools/doc/generate.js

Lines 51 to 52 in e68563e

	} else if (arg.startsWith('--node-version=')) {
	nodeVersion = arg.replace(/^--node-version=/, '');

As far as the release process goes, the release tag is pushed the step before the release is promoted (i.e. made live on the website) so actual releases should be okay: https://github.com/nodejs/node/blob/master/doc/guides/releases.md#14-push-the-release-tag

Anything built from master or a -staging branch though is going to point to a does-not-exist yet version. Wild (untested) idea could be to look for the version passed into the doctool in the changelog (for a release a changelog entry will be created) and if the version isn't in the changelog redirect to a staging branch (if the major is in the changelog) or master (if the major doesn't exist in the changelog).

[aw-davidson]

Thanks @Trott and @richardlau for guiding me through my first commit. I'm also happy to work on this more if you have any ideas / improvements.

[Trott]

Thanks @Trott and @richardlau for guiding me through my first commit. I'm also happy to work on this more if you have any ideas / improvements.

Thanks for the contribution! richardlau's idea here is probably worth experimenting with to see if it would work as an enhancement:

Anything built from master or a -staging branch though is going to point to a does-not-exist yet version. Wild (untested) idea could be to look for the version passed into the doctool in the changelog (for a release a changelog entry will be created) and if the version isn't in the changelog redirect to a staging branch (if the major is in the changelog) or master (if the major doesn't exist in the changelog).