Broken links within documentation

twifty commented 6 years ago

The more I use the docs, the more frustrated I become due to the sheer number of broken/missing links.

For example, on the https://atom.io/docs/api/v1.19.7/Point page, none of the links point to the source code files. Clicking them leads to a 404. While this is one example, there are many many more broken links scattered throughout the docs.

From what I understand, the documentation is auto generated from the source. Would it not be possible to add a build hook to validate these links before the pages are published?

50Wliu commented 6 years ago

For some reason the links are pointing to v13.2.0-12, which doesn't exist. Only v13.2.0-11 does. This has happened before: atom/atom.io#83

jasonrudolph commented 6 years ago

Thanks for reporting this issue. :bow:

tl;dr I think this issue is caused by an npm package version being published without a corresponding git tag being pushed to github.com. I've attempted to explain these findings below. There are three questions I hope to answer. I'm focusing on the first one for now, and then I'll circle back around to the other two.

[x] Can we fix the broken links that we know about?
[x] What other links are broken?
[ ] How do we prevent similar broken links in future releases?

Findings

For some reason the links are pointing to v13.2.0-12, which doesn't exist. Only v13.2.0-11 does.

I see that text-buffer 13.2.0-11 and 13.2.0-12 both exist in the npm registry [A], but for some reason, there's a git tag for v13.2.0-11 and no git tag for v13.2.0-12.

Since Atom 1.19.7 depends on text-buffer v13.2.0-12, the docs link to text-buffer's v13.2.0-12 tag on github.com (https://github.com/atom/text-buffer/blob/v13.2.0-12/src), but no such tag exists:

This has happened before: atom/atom.io#83

In the case of #83, Atom.1.8.0 depends on pathwatcher ~6.2. At the time that Atom 1.8.0 was released, I suspect that pathwatcher 6.2.5 was the latest 6.2.x version, so the docs link to pathwatcher's v6.2.5 tag on github.com (https://github.com/atom/node-pathwatcher/blob/v6.2.5/src), but no such tag exists.

Both cases appear to be caused by an npm package version being published without a corresponding git tag being pushed to github.com. This leads me to ask a few questions:

Can we fix the broken links that we know about?
What other links are broken?
How do we prevent similar broken links in future releases?

I'll start with the first one, and I'll address the other questions in follow-up comments.

Can we fix the broken links that we know about?

If we could retroactively create the missing git tags for text-buffer 13.2.0-12 and pathwatcher 6.2.5, that should fix these links.

text-buffer 13.2.0-12

$ npm show text-buffer@13.2.0-12 | grep gitHead
  gitHead: '5c83383eba5c221d0d0341d811769e2b6f207b21',

@maxbrunsfeld: It looks like I should tag https://github.com/atom/text-buffer/commits/5c83383eba5c221d0d0341d811769e2b6f207b21 as v13.2.0-12. Does that sound right to you?

pathwatcher 6.2.5

$ npm show pathwatcher@6.2.5 | grep gitHead
  gitHead: '8336f8d085d7edab7e7314ee394c2a990913860d',

It doesn't look like this commit exists on github.com (https://github.com/atom/node-pathwatcher/commit/8336f8d085d7edab7e7314ee394c2a990913860d). Maybe this commit was used to release pathwatcher 6.2.5 but the commit was never pushed to github.com?

I'll see if I can fetch the tarball from npm and retroactively push a tag with 6.2.5's content to github.com.

[A] Relevant text-buffer versions on npm:

$ npm show text-buffer | grep 13.2.0-11
 '13.2.0-11',
 '13.2.0-11': '2017-08-30T09:21:46.271Z',

$ npm show text-buffer | grep 13.2.0-12
 '13.2.0-12',
 '13.2.0-12': '2017-08-31T18:38:26.116Z',

jasonrudolph commented 6 years ago

It doesn't look like this commit exists on github.com (atom/node-pathwatcher@8336f8d). Maybe this commit was used to release pathwatcher 6.2.5 but the commit was never pushed to github.com?

I'll see if I can fetch the tarball from npm and retroactively push a tag with 6.2.5's content to github.com.

https://github.com/atom/node-pathwatcher/commit/e92210438d3cf1f14c655acf1b748ffb4ff1d1e8 attempts to create a commit that we can tag as v6.2.5. The commit message describes the approach used to create the commit. Before creating that tag, I'd like to get a sanity check on the approach.

@maxbrunsfeld: Since you might already have some context on this issue due to me pinging you above about text-buffer, would you be willing to take a look at https://github.com/atom/node-pathwatcher/commit/e92210438d3cf1f14c655acf1b748ffb4ff1d1e8 and let me know whether it seems reasonable to tag that commit as v6.2.5?

50Wliu commented 6 years ago

@jasonrudolph it looks like atom/node-pathwatcher@e922104 accidentally removed the Coffeescript files.

jasonrudolph commented 6 years ago

it looks like atom/node-pathwatcher@e922104 accidentally removed the Coffeescript files.

@50Wliu: Good catch!

The tarball on npm (https://registry.npmjs.org/pathwatcher/-/pathwatcher-6.2.5.tgz) doesn't include the CoffeeScript files. 🙁

I wonder if there's another way to track down the original commit that actually created pathwatcher 6.2.5. As far as I can tell, that commit (8336f8d085d7edab7e7314ee394c2a990913860d) was never pushed to github.com, and I'm not sure how to see who published 6.2.5 to npm. 🤔

@zcbenz @kevinsawicki @nathansobo @maxbrunsfeld: Do any of y'all happen to have a 8336f8d085d7edab7e7314ee394c2a990913860d in your local clone of atom/node-pathwatcher?

$ cd ~/github/node-pathwatcher

$ git show 8336f8d085d7edab7e7314ee394c2a990913860d
fatal: bad object 8336f8d085d7edab7e7314ee394c2a990913860d

If so, can you please push it up to a branch on atom/node-pathwatcher and let me know?

maxbrunsfeld commented 6 years ago

It looks like I should tag https://github.com/atom/text-buffer/commits/5c83383eba5c221d0d0341d811769e2b6f207b21 as v13.2.0-12. Does that sound right to you?

:+1:

I wonder if there's another way to track down the original commit that actually created pathwatcher 6.2.5. As far as I can tell, that commit (8336f8d085d7edab7e7314ee394c2a990913860d) was never pushed to github.com, and I'm not sure how to see who published 6.2.5 to npm.

It looks like the link has been fixed ever since Atom 1.9.0, which came out a year and a half ago, and there have been over 20 releases since then, I think it is probably ok to leave this as is.

maxbrunsfeld commented 6 years ago

How do we prevent similar broken links in future releases?

The text-buffer case occurred because I accidentally left Atom's text-buffer dependency pointing at a prerelease version of text-buffer after merging some atom/text-buffer pair of PRs. I have a habit of not running git push --tags when creating pre-releases of npm modules because I create a ton of these prereleases and I felt like all of the prerelease tags add a bunch of undesired autocompletions when using git. Maybe the answer is to just keep and push the tags?

And/or maybe we should add a task to our CI that checks that the generated docs don't have any broken links?

jasonrudolph commented 6 years ago

It looks like I should tag https://github.com/atom/text-buffer/commits/5c83383eba5c221d0d0341d811769e2b6f207b21 as v13.2.0-12. Does that sound right to you?

:+1:

Thanks, @maxbrunsfeld. I created that tag (https://github.com/atom/text-buffer/releases/tag/v13.2.0-12) and that fixes the broken links in the Atom 1.19.7 docs that point to the text-buffer source code.

I wonder if there's another way to track down the original commit that actually created pathwatcher 6.2.5. As far as I can tell, that commit (8336f8d085d7edab7e7314ee394c2a990913860d) was never pushed to github.com, and I'm not sure how to see who published 6.2.5 to npm.

It looks like the link has been fixed ever since Atom 1.9.0, which came out a year and a half ago, and there have been over 20 releases since then, I think it is probably ok to leave this as is.

Fair enough. I'll treat the Atom 1.8.0 broken doc links as wontfix, and I'll instead focus on how to prevent this issue from recurring in future releases.

jasonrudolph commented 6 years ago

What other links are broken?

I ran linkchecker against the docs for the latest stable release (https://atom.io/docs/api/v1.23.3). It identified broken links for the following URLs:

If you're interested, you can see the raw output in this gist, including info about which pages include links to these URLs. I'll see what we can do about fixing these broken links.

50Wliu commented 6 years ago

In addition, Repository was fixed by https://github.com/atom/atom/pull/16570.

Most of the rest like Marker, NotificationElement, DisplayLayer, etc. are 404ing because they're referencing classes that haven't been added to the public API.

jasonrudolph commented 6 years ago

I don't know why the KeymapManager URLs are 404ing ...

@50Wliu: It was due to the same problem we saw with text-buffer above: atom-keymap 8.2.8 was published to npm, but there was no corresponding git tag for atom-keymap 8.2.8. I tracked down the SHA associated with atom-keymap 8.2.8 and tagged it as v8.2.8. Those URLs are no longer 404ing. :sweat_smile:

... I went and fixed some of the other broken URLs:

@50Wliu: Thank you! :zap:

Most of the rest like Marker, NotificationElement, DisplayLayer, etc. are 404ing because they're referencing classes that haven't been added to the public API.

@50Wliu: Thanks for confirming that.

I built Atom from master (at https://github.com/atom/atom/commit/8e2a8b15f8f36618f424a27e7139c163a9722c6e) and used the resulting atom-api.json file to generate the docs using my local instance of atom.io. linkchecker identified 44 broken links in the generated docs. I'm looking into atom.io's logic for generating docs to see if we can improve the way it handles references to classes that aren't part of the public API, and hopefully that will resolve the majority of these broken links. 🤞

jasonrudolph commented 6 years ago

Whoops. This got automatically closed due to the content of the pull request body in atom/atom#16617:

Update text-buffer to 13.11.6 to pull in documentation fixes: https://github.com/atom/atom.io/issues/134#issuecomment-358485697

jasonrudolph commented 6 years ago

I went and fixed some of the other broken URLs...

@50Wliu: Both the master branch and the 1.24-releases branch (https://github.com/atom/atom/compare/608d3af2d45dba94dadb98394d1334519a9e512e...6567653418a5a3ecccea28a7010042c02d92ae87) now include these fixes. As a result, they'll be included in the upcoming Atom 1.24 release.

Thanks again for these updates! :zap:

jasonrudolph commented 6 years ago

With @50Wliu's fixes above and with the latest improvements to atom.io, I anticipate that all the broken links will be fixed when the docs are published for the upcoming 1.24 release. With that in mind, I'm going to close this issue.

If you continue to see any broken links in the 1.24 release (or in future releases), please let us know. :bow:

atom / atom.io