Stabilise disable minification #56359
Conversation
|
What is the use case for this option? Surely if people want to see the none minified versions they can just look at rustdoc's source code. |
|
Well, rustdoc messes with JS content by adding stuff in it. So it's not that simple to get a view of what's really generated unless you disable the minification. |
|
Rustdoc also minifies any user-supplied themes as well as the search index. It also adds an extra line to This is a debugging flag that @GuillaumeGomez uses personally, and the original PR for this was among a handful of no-comment stabilization PRs that he started at the same time, alongside threads for It looks like you've added docs since the original PR, but i'm not aware of any tests in this repo for the minifier functionality. (How much is the Regardless, we should hold an FCP before merging this, since it's a feature stabilization. |
|
Testing is complicated directly on rustdoc since everytime there is an update, we'll have to update the minification tests as well. I added tests (maybe not enough?) in the minifier crate directly. I'll let you judge if there are enough, and if not, I'll add even more! :D |
|
☔ The latest upstream changes (presumably #57063) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage, @QuietMisdreavus and @GuillaumeGomez, any progress? |
|
Well, I'm still waiting for a definitive opinion. I'll rebase though. |
06ae169 to
3ae3f49
Compare
|
Need to check on gtk-rs if cargo workspaces could fix my issue. |
|
We discussed this at the Rust All-Hands. @GuillaumeGomez discussed a use case for this in gtk-rs, where there were difficulties in writing all the docs at once. However, it seems like putting them into a Cargo workspace could solve the issue, so imperio needs to investigate that. If that fixes that issue, the only remaining use case is debugging, and it's not pressing to have a debugging flag stabilized. If that's the only remaining use, i would move to close this PR. |
|
☔ The latest upstream changes (presumably #58361) made this pull request unmergeable. Please resolve the merge conflicts. |
3ae3f49 to
2252577
Compare
|
ping from triage @QuietMisdreavus waiting for your review on this |
|
I still want to see whether @GuillaumeGomez was able to solve the issues in gtk-rs with workspaces instead of using this flag. If workspaces work for gtk-rs, then i don't think this needs to be stabilized. |
|
I didn't take time to do it yet. >< |
|
☔ The latest upstream changes (presumably #59050) made this pull request unmergeable. Please resolve the merge conflicts. |
2252577 to
ccc88a2
Compare
|
Ping from triage @GuillaumeGomez: What is the status of this PR? |
|
I still need to check on gtk-rs... |
|
ping from triage @GuillaumeGomez any updates? is this blocked on anything? |
|
I still need to check the workspace story but it'll need to wait for a gtk-rs update which is taking longer than expected. |
|
Team member @QuietMisdreavus has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
@QuietMisdreavus I applied the changes you suggested. |
|
I was able to write a small test to ensure that the // compile-flags:--disable-minification
// @has search-index.js
// @!has - 'var R'
/// The quick brown fox jumps over the lazy dog.
pub struct SomeStruct;
/// The quick brown fox jumps over the lazy dog.
pub fn some_fn() {}This just checks for the |
|
I'd like to see if @ollie27 wants to weigh in, but otherwise, if you add that test it LGTM. |
|
I'll add your test. |
|
Test added! |
|
Thanks! Now i just want to wait for ollie27's comment, and we can ride the FCP out. |
|
The issues with the minifier stem from the fact that it is run on every invocation of rustdoc, instead of after all the documentation has been collected. For example, since the JavaScript minification only adds to the minified string index (the Even with this fixed, there's still a lot of useless work going on per-invocation. A simple solution to both issues would be to wait for all the rustdoc invocations to finish, and then run the minifier once over all the artifacts. However, I think this would require However, if minification happened after I understand that saving bandwidth is important, especially to users who have poor internet connections. But, I'm skeptical that minification is |
|
☔ The latest upstream changes (presumably #62766) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@euclio: Considering the size of the |
|
I think over time i've come around to @euclio's position. I'm aware that rustdoc generates a large file for the search index, but that doesn't stop it from being a performance bottleneck or occasionally generating faulty or inconsistent JS. I believe our niche as a tool doesn't need to include making sure that we serve optimally-small resources. If some site hosting docs wishes to shrink the resources it loads, it can minify and/or gzip them after the fact. This means we waste less time in the average case (user generating local docs for themselves). I feel like the best place for the minifier is as an external tool that can be called as a separate script - |
|
Closing this after a discussion with @GuillaumeGomez |
|
@rustbot modify labels to -S-inactive +S-inactive-closed |
Fixes #54819.
Reopening of #54827.
r? @QuietMisdreavus