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

Implement URL redirecting #1237

Merged
merged 10 commits into from
May 30, 2020
Merged

Implement URL redirecting #1237

merged 10 commits into from
May 30, 2020

Conversation

Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan commented May 26, 2020

This is a fairly straightforward implementation based on the comments from #430.

Fixes #430, CC #1235.


Some potential future work would be allowing the user to configure redirects inline with a [output.html.redirect] table in book.toml or reading from a key = value file discovered by having some sort of redirect = "redirects.txt" under the [output.html] table.

This is easy enough to implement in a backwards-compatible way (backwards-compatible for end users, it would require a library change) by swapping the HashMap<String, String> for some RedirectsMap enum that is either a HashMap or PathBuf, and giving the RedirectsMap a load_redirects() -> Result<impl Iterator<Item = (String, String)>, Error> method.

I've decided to skip that for now, seeing as it's not really needed for @XAMPPRocky's use case and seems like overkill.

Copy link
Member

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Thanks for writing this @Michael-F-Bryan !

- **redirect:** A subtable used for generating redirects when a page is moved.
The table contains key-value pairs where the key is the path to the moved
page, relative to the build directory and the value can be an ordinary URL
or the *absolute* path to another page in the book
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why this emphasizes an absolute path. If there is a redirect "foo/bar.html" = "relative.html", I believe that will redirect to foo/relative.html correctly, will it not? Can you say more why it says absolute here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading through that again, I think I got my logic back-to-front when writing the docs.

We want the pages which were moved (the bit before =) to be specified using absolute paths so there's no ambiguity over which file inside the build dir we're talking about. (i.e. it starts with /, therefore it's relative to the renderer's build directory)

You're correct about the redirect target (the bit after =) needing no constraints. We're just substituting the value into <meta http-equiv="refresh" content="0;URL='{{url}}'"> and the browser would take care of everything for us, so pretty much anything you'd use in a normal markdown link is fair game.

@ehuss
Copy link
Contributor

ehuss commented May 29, 2020

Awesome, thanks!

I imagine another enhancement would be to support # anchor redirects. I've used these heavily when splitting chapters up (like https://github.com/rust-lang/reference/blob/master/src/types-redirect.html).

@Michael-F-Bryan
Copy link
Contributor Author

I imagine another enhancement would be to support # anchor redirects. I've used these heavily when splitting chapters up (like https://github.com/rust-lang/reference/blob/master/src/types-redirect.html).

Can you explain the logic behind this, and how it's intended to work? It looks like you're handling the case where a user follows a link to a particular section on a particular page (e.g. /index.html#overview) and the section has been moved to its own section.

I like the idea, but feel the implementation would be a lot more involved and out of scope for this PR. The way we prepare data for generating normal chapters is a bit of a code monster, so updating existing code to thread a chapter's specific redirect table through to Handlebars would be a lot more complex than tacking a new stage on the end, like we did here.

@ehuss
Copy link
Contributor

ehuss commented May 30, 2020

Yea, it can definitely be done later. I might do it if I feel motivated.

This works in the same way, just that it is inspecting the anchor instead of being an unconditional redirect. For example, https://doc.rust-lang.org/reference/types.html#boolean-type redirects to the new page (https://doc.rust-lang.org/reference/types/boolean.html). This was once one large page that got split up into individual pages. There were a lot of links in the ecosystem to these headers, and I didn't want to have them go to the wrong place. The Cargo book might be a better example, where sections got moved to all sorts of different locations.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

Fixed a small typo in the TOML example.

I'm not sure I think it is really important to emphasize that the source path is absolute (it's a little confusing to talk about an absolute path that is actually relative to another). But since it accepts both, we can tweak the docs later if anyone else seems confused.

@ehuss ehuss merged commit b375f4e into rust-lang:master May 30, 2020
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
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.

Facilitate maintaining URLs with redirect mapping
3 participants