Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add dynamic source code links #33996

Closed

Conversation

aw-davidson
Copy link
Contributor

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 21, 2020
@Trott
Copy link
Member

Trott commented Jun 21, 2020

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

Trott commented Jun 21, 2020

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 aw-davidson changed the title doc: add source code links under module headings doc: add dynamic source code links Jun 22, 2020
@aw-davidson
Copy link
Contributor Author

@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.

Screen Shot 2020-06-22 at 1 36 59 PM

@@ -82,7 +82,7 @@ async function main() {
const content = await unified()
.use(replaceLinks, { filename, linksMapper })
.use(markdown)
.use(html.preprocessText)
.use(html.preprocessText, { nodeVersion })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo the mode change for this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott
Copy link
Member

Trott commented Jun 22, 2020

@nodejs/documentation @nodejs/website

@aw-davidson
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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.

@richardlau richardlau added the review wanted PRs that need reviews. label Jun 25, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2020
@Trott
Copy link
Member

Trott commented Jun 26, 2020

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
Copy link
Member

Trott commented Jun 26, 2020

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
Copy link
Member

Trott commented Jun 26, 2020

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.)

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2020
jasnell pushed a commit that referenced this pull request Jun 26, 2020
Fixes: #33977

PR-URL: #33996
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

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.

@jasnell jasnell closed this Jun 26, 2020
@richardlau
Copy link
Member

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)

} 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
Copy link
Contributor Author

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
Copy link
Member

Trott commented Jul 1, 2020

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).

MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
Fixes: #33977

PR-URL: #33996
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Fixes: #33977

PR-URL: #33996
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Fixes: #33977

PR-URL: #33996
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants