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

Allow configuring rustdoc --disable-minification in config.toml #83059

Merged
merged 2 commits into from
Mar 13, 2021

Conversation

notriddle
Copy link
Contributor

This way, you can debug rustdoc's JavaScript and CSS file with normal F12 Dev Tools and you'll have useful line numbers to work with.

This way, you can debug rustdoc's JavaScript and CSS file
with normal F12 Dev Tools and you'll have useful line numbers
to work with.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Mar 12, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

Would it maybe be more useful to include a source map instead? Then it would always work, and we wouldn't have to keep adding more options to rustdoc.

@jyn514 jyn514 added A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-rustdoc-ui Area: rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 12, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

Oh wait, this is changing bootstrap, not rustdoc. Why does rustdoc even have a --disable-minification option??

@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

It looks like it's still unstable - what do you think about removing it in favor of unconditionally minifying and generating a source map?

cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

I use --disable-minification often. However, I'm not sure if using source maps would be very useful in our case considering it's only minification.

@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

I use --disable-minification often. However, I'm not sure if using source maps would be very useful in our case considering it's only minification.

I don't understand - if you use disable-minification often, why wouldn't you find source maps useful? They do the same thing, but source maps can be included unconditionally instead of having to change the build process.

@GuillaumeGomez
Copy link
Member

Hum, that's actually a good point. Let's go for source maps then!

@notriddle
Copy link
Contributor Author

The big downside is that source map generation is something you need to implement, and the minifier crate doesn't. It's a complicated feature that will take a few person-days to get it good enough to use. And when a minifier has a bug, it can take further hours of diving before you even realize that it's a bug in the minifier and not a bug in the actual main.js code, and fixing them can get pretty complex, because these can be large and complex beasts.

Adding an option to turn off minification took a few minutes, and can be merged right now, and if it has a bug, it'll fail by minifying or not minifying the code when you expect, which will be obvious.

@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

Given that this already exists, I'm ok with adding an option for this to bootstrap (although @Mark-Simulacrum may want you to just use RUSTDOCFLAGS=--disable-minification instead). But I think we shouldn't plan to stabilize this option, and see if we can add source maps to make up for it.

@jyn514 jyn514 changed the title Add a disable-minification option for rustdoc Allow configuring rustdoc --disable-minification in config.toml Mar 12, 2021
@Mark-Simulacrum
Copy link
Member

I think adding an option like this PR does is fine. I think it's reasonable to expose developer-only options via config.toml.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 12, 2021

📌 Commit fdb3e82 has been approved by Mark-Simulacrum

@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 Mar 12, 2021
@Mark-Simulacrum
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 12, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 12, 2021

📌 Commit 095b6d2 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2021
Rollup of 6 pull requests

Successful merges:

 - rust-lang#82984 (Simplify ast block lowering)
 - rust-lang#83012 (Update Clippy)
 - rust-lang#83020 (Emit the enum range assumption if the range only contains one element)
 - rust-lang#83037 (Support merge_functions option in NewPM since LLVM >= 12)
 - rust-lang#83052 (updated vulnerable deps)
 - rust-lang#83059 (Allow configuring `rustdoc --disable-minification` in config.toml)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 684fa19 into rust-lang:master Mar 13, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 13, 2021
@notriddle notriddle deleted the config-toml-disable-minification branch March 13, 2021 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-rustdoc-ui Area: rustdoc UI (generated HTML) 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.

7 participants