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

rustdoc: theme.js may be incorporated into main.js #82616

Closed
jsha opened this issue Feb 28, 2021 · 3 comments · Fixed by #82732
Closed

rustdoc: theme.js may be incorporated into main.js #82616

jsha opened this issue Feb 28, 2021 · 3 comments · Fixed by #82732
Assignees
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jsha
Copy link
Contributor

jsha commented Feb 28, 2021

theme.js is loaded very early in the rustdoc HTML documents, "to reduce theme switch latencies as much as possible:" https://github.com/rust-lang/rust/blob/master/src/librustdoc/html/render/mod.rs#L782-L834. It seems when themes were first introduced in 003b2bc, theme.js was responsible for setting the displayed theme based on local storage. However, since 5f93159, that job is done by storage.js and theme.js is mainly responsible for setting handlers on the UI elements used to pick themes.

I think it's possible now to incorporate theme.js into main.js and reduce the number of fetches per page by 1. @GuillaumeGomez what do you think?

@GuillaumeGomez
Copy link
Member

There are two reasons for this:

  1. This is a completely generated file.
  2. It's small and allow to make the switch much faster than it would if integrated into a (much) bigger file.

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 28, 2021
@jsha
Copy link
Contributor Author

jsha commented Feb 28, 2021

This is a completely generated file.

main.js is also generated, in the sense that it's baked into rustdoc and then emitted and doc build time. rustdoc could concatenate theme.js to the end of main.js at doc build time.

Also, it looks like the only things that are interpolated into theme.js are the theme names. If we wanted to further simplify things, we could put the constant part of theme.js in main.js, and just interpolate an array containing theme names.

It's small and allow to make the switch much faster than it would if integrated into a (much) bigger file.

When you say "make the switch" do you mean the change from the default theme to the one the user has selected? Because as far as I can tell that happens in storage.js, not theme.js. theme.js seems to be all about filling in the buttons that go under the paintbrush icon, to select different themes.

@GuillaumeGomez
Copy link
Member

You're right, my bad. Then yes, it doesn't make much sense to keep it on its own, we can simply merge it within main.js when generating the documentation. Don't know why I was sure it was handling the theme switch...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants