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: move some search code into search.js #84150

Merged
merged 7 commits into from
Apr 17, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Apr 13, 2021

This reduces main.s from 3094 lines to 1587. Also it saves some bytes
of download in the case where search isn't used.

There were a fair number of variables that needed to be accessible in
both main.js and search.js, but I didn't want to put too many symbols in
the global namespace, so I consolidated much of the search-related
state and functions into a new object window.searchState.

Demo at https://hoffman-andrews.com/rust/move-search/std/?search=foo

Export a few variables and functions into the global scope because they
are needed both by main.js and search-index.js.
@rust-highfive
Copy link
Contributor

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Contributor

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2021
@rust-log-analyzer

This comment has been minimized.

jsha added 2 commits April 13, 2021 00:07
This allows sharing across main.js and search.js without exporting too
many symbols into the global namespace.
let mut v = String::from("var searchIndex = JSON.parse('{\\\n");
v.push_str(&all_indexes.join(",\\\n"));
v.push_str("\\\n}');\ninitSearch(searchIndex);");
let v = static_files::SEARCH_JS
Copy link
Member

Choose a reason for hiding this comment

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

Please don't make this change! search-index.js is only supposed to contain the search "content", nothing else. Create a new JS file or insert the content into main.js but don't change how we handle search-index.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! Happy to split this out into search.js. Why is search-index.js supposed to only contain the search content and nothing else?

Copy link
Member

Choose a reason for hiding this comment

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

Multiple reason: this is a file different for every crate, meaning that docs.rs cannot store it once and for all. Also it's faster to parse a smaller file (even if here, the part you add might not be that big compared to the search index itself).

So here, it's mostly for docs.rs reason.

Copy link
Member

Choose a reason for hiding this comment

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

And for docs.rs it would be nice to convert it to .json at some point 😁

Copy link
Member

Choose a reason for hiding this comment

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

Not possible as is unfortunately, you can load json files for local docs. :3

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

The end result is very nice, thanks a lot! Just one thing to check first: @rust-lang/docs-rs there is new static JS file added. iirc, there is nothing to be done anymore on docs.rs side now but just want to check before approving this PR.

@jyn514
Copy link
Member

jyn514 commented Apr 14, 2021

here is new static JS file added. iirc, there is nothing to be done anymore on docs.rs side now but just want to check before approving this PR.

Yes, that's fine. You don't need to keep pinging :)

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 14, 2021

Yes, that's fine. You don't need to keep pinging :)

Promise, it was the last time! :)

Co-authored-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
@jsha
Copy link
Contributor Author

jsha commented Apr 14, 2021

Thanks for the review! Should I rebase?

@jyn514
Copy link
Member

jyn514 commented Apr 14, 2021

@jsha no need, the PR doesn't have conflicts. If people had to rebase on every merge we'd never get any PR merged 😆

@GuillaumeGomez
Copy link
Member

Thanks! r=me once the CI is happy. (If you want to rebase, no one will stop you, but it's not required as @jyn514 explained ;) )

@jsha jsha changed the title rustdoc: move search some code into search.js rustdoc: move some search code into search.js Apr 17, 2021
@jsha
Copy link
Contributor Author

jsha commented Apr 17, 2021

Looks like CI is done. :-)

@jyn514
Copy link
Member

jyn514 commented Apr 17, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Collaborator

bors commented Apr 17, 2021

📌 Commit 6f1f3eb has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2021
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2021
Rollup of 4 pull requests

Successful merges:

 - rust-lang#83237 (rustdoc: use more precise relative URLs)
 - rust-lang#84150 (rustdoc: move some search code into search.js)
 - rust-lang#84203 (rustdoc: Give a more accurate span for anchor failures)
 - rust-lang#84257 (Add documentation to help people find `Ipv4Addr::UNSPECIFIED`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b5ce9c4 into rust-lang:master Apr 17, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 17, 2021
@jsha jsha deleted the defer-search-js branch April 18, 2021 00:21
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 4, 2023
…tate-focus, r=GuillaumeGomez

docs: fix broken link "search bar"

Regression introduced by rust-lang#84150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants