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

Load the sidebar toc from a shared JS file or iframe #2414

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Jul 16, 2024

Before this change, the Rust unstable-book is 88MiB. With this change, it becomes 15MiB. Other pages might not be as extreme, but it's expected to help any book like this.

This change is so drastic because, if every chapter has a link to every other chapter, the result is O(n2) text output.

Related to : https://rust-lang.zulipchat.com/#narrow/stream/266220-t-rustdoc/topic/Tracking.20overall.20size/near/431831262

Preview: http://notriddle.com/rustdoc-html-demo-9/on2-book/index.html

Loading filmscripts (green line is first contentful paint):

Desktop

image

Mobile

image

Before this change, the Rust `unstable-book` is 88MiB.
With this change, it becomes 15MiB. Other pages might not be
as extreme, but it's expected to help any book like this.

This change is so drastic because, if every chapter has a link to
every other chapter, the result is *O*(n<sup>2</sup>) text output.
@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Jul 16, 2024
@GuillaumeGomez
Copy link
Member

This is awesome, thanks!

@GuillaumeGomez
Copy link
Member

I have merge rights but I'm not sure I'm supposed to do it. I'll ask the dev-tools team.

@notriddle
Copy link
Contributor Author

It’s not just that. The unresolved question is if making the sidebar dependent on JS is okay.

@GuillaumeGomez
Copy link
Member

That's something that was mentioned on zulip. Not a maintainer so only my point of view: we can still browse the book without JS (although less efficiently), so the regression is "ok". Maybe we can have a middle-ground where only the big sections are provided by default and the rest is filled by JS?

@notriddle
Copy link
Contributor Author

Another option: include the TOC in index.html, but require JS for every other page.

@GuillaumeGomez
Copy link
Member

That however would be pretty bad.

Uses an iframe instead. The downside of iframes comes from them
not necessarily being same-origin as the main page (particularly
with `file:///` URLs), which can cause themes to fall out of sync,
but that's not a problem here since themes don't work without JS
anyway.
@notriddle
Copy link
Contributor Author

Second commit has another solution to that problem: use an iframe. Those work without JavaScript.

I wind up using it as a fallback, instead of replacing the JS solution, because of Same-Origin-Policy shenanigans. Running out of a file:// URL will result in toc.html having separate localstorage from the main page, which isn't a problem when JS is disabled, but when it's enabled it would result in unsynchronized themes.

@notriddle
Copy link
Contributor Author

r? @ehuss

@notriddle notriddle changed the title Load the sidebar toc from a shared JS file Load the sidebar toc from a shared JS file or iframe Oct 1, 2024
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! I'm a little nervous about merging this since it is such a large change, and the added complexity is hard for me to maintain. I've also long felt that mdbook makes far too many requests (js and css files, etc.), and I am uncertain how this affects caching or performance in various scenarios. Another concern might be whether crawlers will be able to navigate properly. I'm also a little concerned over the amount of duplication in toc.html.hbs, which I feel will very likely get out of sync, or not get any testing.

However, I'll go ahead and merge with the understanding that you'll help with any issues, or else this is something that we can try to revert if necessary.

@ehuss ehuss added this pull request to the merge queue Nov 2, 2024
Merged via the queue into rust-lang:master with commit 271bbba Nov 3, 2024
11 checks passed
@notriddle notriddle deleted the on2 branch November 3, 2024 02:07
@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2024

@notriddle This seems to have broken a few things with folded sidebar entries. The folding doesn't work (clicking on the chapter heading doesn't expand it, and causes some rendering flashes). In some cases where I am able to get the folds to open, all the links are broken.

Some example books would be https://github.com/rust-lang/rustc-dev-guide or https://github.com/rust-lang/rust-by-example/.

Do you think you would be able to look at that?

@GuillaumeGomez
Copy link
Member

The broken books are online already or not?

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2024

No, you need to check out those repositories and use the latest mdbook in them.

Or, just enable the fold option in any book.

@GuillaumeGomez
Copy link
Member

Ok, if @notriddle didn't answer by tomorrow, I'll take a look.

@d3v-null
Copy link

This change has broken how mdbook handles links in a sidebar.

e.g. if my summary / sidebar is

# Summary
[Introduction](README.md)
# User Guide
- [Introduction](user/intro.md)
- [Plot solutions](user/plotting.md)

in v0.4.40, the sidebar link will be given relative to the page you're on. if I'm on the user/intro.md page, then the sidebar href for the plotting link will be ../user/plotting.html , but after v0.4.41 where this change was released, there is no longer a relative link, instead, the href is user/plotting.html

@GuillaumeGomez
Copy link
Member

We need to generate a sidebar in each "folder" then.

@shenef
Copy link

shenef commented Jan 23, 2025

That may be the same issue as #2477 and #2482. It's very easily missed that the index.hbs file has to be updated.

@d3v-null
Copy link

thanks @shenef , unfortunately the person who set up mdbook is no longer around so it's not clear to me how to update index.hbs in my repo https://github.com/MWATelescope/mwa_hyperdrive

I tried mdbook init , which does update index.hbs but results in blank pages with mdbook serve

@d3v-null
Copy link

never mind, the above works, there was something else using that port.

michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this pull request Jan 31, 2025
  - reran mdbook init --theme
  - keep playground improvements
  - keep language selector and suggest edit / edit to translation button text
  - improvement: toc is in separate js file, makes html file way smaller

relevant upstream changes that are used:
- rust-lang/mdBook#2414
- rust-lang/mdBook#2421
- rust-lang/mdBook#2454
- rust-lang/mdBook#2463
michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this pull request Jan 31, 2025
  - reran mdbook init --theme
  - keep playground improvements
  - keep language selector and suggest edit / edit to translation button text
  - improvement: toc is in separate js file, makes html file way smaller

relevant upstream changes that are used:
- rust-lang/mdBook#2414
- rust-lang/mdBook#2421
- rust-lang/mdBook#2454
- rust-lang/mdBook#2463
d3v-null added a commit to MWATelescope/mwa_hyperdrive that referenced this pull request Feb 2, 2025
michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this pull request Feb 5, 2025
  - reran mdbook init --theme
  - keep playground improvements
  - keep language selector and suggest edit / edit to translation button text
  - improvement: toc is in separate js file, makes html file way smaller
  - additionally updated to mdbook-i18n-helpers-0.3.5

relevant upstream changes that are used:
- rust-lang/mdBook#2414
- rust-lang/mdBook#2421
- rust-lang/mdBook#2454
- rust-lang/mdBook#2463
michael-kerscher added a commit to google/comprehensive-rust that referenced this pull request Feb 7, 2025
mdbook in CI pipeline is updated to mdbook-0.4.44
  - reran mdbook init --theme
  - keep playground improvements
- keep language selector and suggest edit / edit to translation button
text
  - improvement: toc is in separate js file, makes html file way smaller
  - additionally updated to mdbook-i18n-helpers-0.3.5

relevant upstream changes that are used:
- rust-lang/mdBook#2414
- rust-lang/mdBook#2421
- rust-lang/mdBook#2454
- rust-lang/mdBook#2463
michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this pull request Feb 7, 2025
mdbook in CI pipeline is updated to mdbook-0.4.44
  - reran mdbook init --theme
  - keep playground improvements
- keep language selector and suggest edit / edit to translation button
text
  - improvement: toc is in separate js file, makes html file way smaller
  - additionally updated to mdbook-i18n-helpers-0.3.5

relevant upstream changes that are used:
- rust-lang/mdBook#2414
- rust-lang/mdBook#2421
- rust-lang/mdBook#2454
- rust-lang/mdBook#2463
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants