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

detect broken links #1698

Merged
merged 7 commits into from
Sep 30, 2024
Merged

detect broken links #1698

merged 7 commits into from
Sep 30, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Sep 30, 2024

Currently only warning, but we could make it break the build like we do in Plot's documentation. Should this be a build option?

closes #363
closes #1683

(Test by running yarn docs:build. Merge #1699 to see the happy state.)

happy
happy

unhappy
unhappy

@Fil Fil requested a review from mbostock September 30, 2024 14:06
@Fil Fil mentioned this pull request Sep 30, 2024
src/build.ts Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor Author

Fil commented Sep 30, 2024

The page sizes table we show at the end and the link checker both look similar in that they are "reporters" rather than strictly necessary for the build.

I put the link checker at this location because it's fast and if it must break the build it's better to do it sooner. But I'd prefer the happy report to be shown at the end.

src/build.ts Outdated Show resolved Hide resolved
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Hooray! Great to have this.

I made some changes. To summarize:

  • I encapsulated the code a bit more, reducing the number of local variables in the already quite large and stateful build function. It would be nice to have separate unit tests for validateLinks, perhaps, but it’s pretty simple so not urgent.
  • I moved the broken link warning to the end where it’s more likely to be seen. If we want to make this a hard error, then we can move it earlier.
  • I renamed links to localLinks to indicate that we’re only extracting (and validating) local links, not all links.
  • I added encoding as appropriate (${path}#${encodeURIComponent(id)}).
  • I skipped validation of <a href download> since these links are rewritten to point to files.
  • I disallowed (non-download) links to files, since these links are not rewritten and would be broken.
  • I would like to allow links to exported files, but I was lazy and didn’t implement this yet.
  • I allowed links to exported modules (which is unlikely but should work).
  • I added normalization for trailing .html (in addition to the existing /index).
  • I used parseRelativeUrl instead of hacking URL with proto:….
  • I fixed a longstanding quirk in resolvePath (and added tests).
  • I added unit tests for the new findAssets logic.

@mbostock mbostock merged commit ca823ce into main Sep 30, 2024
4 checks passed
@mbostock mbostock deleted the fil/detect-broken-links branch September 30, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

case sensitive page URLs Detect broken links during build
2 participants