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,tools: checkLinks.js does not cover api #35189

Closed
DerekNonGeneric opened this issue Sep 14, 2020 · 2 comments · Fixed by #35191
Closed

doc,tools: checkLinks.js does not cover api #35189

DerekNonGeneric opened this issue Sep 14, 2020 · 2 comments · Fixed by #35191
Labels
build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Comments

@DerekNonGeneric
Copy link
Contributor

This should be tested when building the HTML, but apparently it only tests links internal to all.html page:

const hrefRe = / href="#(\w+)"/g;
while (match = hrefRe.exec(all)) {
if (!ids.has(match[1])) throw new Error(`link not found: ${match[1]}`);
}

Usually that would cover all links internal to the docs, as all links to doc pages are stripped from the filename to keep only the hash part:

apicontent += data.slice(match.index + match[0].length)
.replace(/<!-- API END -->[\s\S]*/, '')
.replace(/<a href="(\w[^#"]*)#/g, (match, href) => {
return htmlFiles.includes(href) ? '<a href="#' : match;
})
.trim() + '\n';

But in this case, because modules_module.html doesn't exist, it's treated as an external page and the broken links slip through the test…

I haven't dug into why but it looks like the api docs are deliberately excluded

Yes indeed. The reason checkLinks.js does not cover api is because we are using .html extension to reference other doc pages, while checkLinks.js would expect .md. One way of fixing it would be to use .md extensions in the Markdown files, and use checkLinks.js to test said links – it would also improve the experience of navigating the docs through GitHub web UI. Another way would be to tweak the current test to make sure this doesn't reproduce.

Originally posted by @aduh95 in #35182 (comment)

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Sep 14, 2020

Opened this issue using @aduh95's preliminary findings as seed. I'd like for it to track progress on the unreliability of the link checker and discuss the intended fix as a potential feature request.

One way of fixing it would be to use .md extensions in the Markdown files, and use checkLinks.js to test said links – it would also improve the experience of navigating the docs through GitHub web UI.

For those who haven't been following previous discussions regarding expectations we have about the docs, it's pretty clear that the fix described in the above quote should be the one pursued. For context, here is one excerpt supporting this notion.

One downside to this approach is that it further degrades the experience of browsing the markdown files on GitHub. Not necessarily a deal-breaker, but the upside should outweigh the downside.
#32916 (comment)

I was not previously aware of the Markdown documents' links not functioning on github.com, but that should probably be pretty high on the list of feature requests. From what I've observed elsewhere, it's highly desirable since a lot of docs PRs are made using the GitHub UI (esp. for small typos), so being able to navigate the docs this way is seen as being of high value.

I'm a bit crunched for time this week, but it would be great to get some idea of how difficult this would be to accomplish. Aside from simply changing the file name extensions and associated links, I'd be interested in knowing if there's anything else to be aware of (any hard-coded file extensions used in the Makefile, doctool JS source, etc.).

/cc @Trott @GeoffreyBooth @aduh95 @richardlau

@DerekNonGeneric DerekNonGeneric changed the title checkLinks.js does not cover api doc,tools: checkLinks.js does not cover api Sep 14, 2020
@DerekNonGeneric DerekNonGeneric added doc Issues and PRs related to the documentations. build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. tools Issues and PRs related to the tools directory. labels Sep 14, 2020
@aduh95
Copy link
Contributor

aduh95 commented Sep 14, 2020

I have a fix for that in #35191, it just needs #35182 to land first. The most difficult part of it will be not to get drown in all the merge conflicts, as this touches every single files in doc/api.

Trott pushed a commit to aduh95/node that referenced this issue Oct 1, 2020
Refs: nodejs#35244

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott pushed a commit to aduh95/node that referenced this issue Oct 1, 2020
Fixes: nodejs#35189

PR-URL: nodejs#35191
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott closed this as completed in ecf5060 Oct 1, 2020
danielleadams pushed a commit that referenced this issue Oct 6, 2020
This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

PR-URL: #35191
Fixes: #35189
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this issue Oct 6, 2020
Refs: #35244

PR-URL: #35191
Fixes: #35189
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this issue Oct 6, 2020
Fixes: #35189

PR-URL: #35191
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Refs: nodejs#35244

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Fixes: nodejs#35189

PR-URL: nodejs#35191
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants