Skip to content

Book 2nd edition is broken on stable and beta #44704

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.

Sign up for GitHub

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

Jump to bottom
Closed
steveklabnik opened this issue Sep 19, 2017 · 18 comments
Closed

Book 2nd edition is broken on stable and beta #44704

steveklabnik opened this issue Sep 19, 2017 · 18 comments
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug.

Comments

@steveklabnik
Copy link
Member

steveklabnik commented Sep 19, 2017 •
edited
Loading

If this bug affects you, please see the workaround here: #44704 (comment)

The current stable and beta docs are missing a single javascript file, store.js.

Stable rust builds with mdbook=0.0.22 In 0.0.23, mdbook introduced a new javascript file, store.js. The Book second edition uses a custom template in order to display the "this is a draft" banner, and so, we updated it to include the new file rust-lang/book#783. However, this was wrong; it shouldn't have been done until mdbook was updated in the main repository. As such, things are now very broken:

book

As nightly has updated mdbook, this has been fixed, but having the main way of learning Rust broken for ~10 weeks isn't great.

For stable

We could fix the online docs by just uploading store.js to the appropriate place. I am normally very against changing what we distribute after the fact, but simply adding a file seems very minimal.

While this regression is in some sense very serious, I don't think it's worth cutting a new stable for, with this workaround.

For beta

In my opinion, we should send in a backport fix to beta. This involves upgrading mdbook on beta.

Tagging as "regression from stable to nightly" even though it's been fixed in nightly; this is about what to do for stable and beta. Tagging @rust-lang/infra for the stable fix, tagging @rust-lang/docs for the beta fix.
@steveklabnik steveklabnik added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 19, 2017
@carols10cents
Copy link
Member

We could also remove the line that was added in https://github.com/rust-lang/book/pull/783/files from all the HTML files for the book for all the versions...... which is arguably "more correct", but also more work.

@carols10cents
Copy link
Member

@steveklabnik have you confirmed that adding the store.js file to a book built with mdbook 0.22 fixes the styling?

@steveklabnik
Copy link
Member Author

We could also remove the line that was added in https://github.com/rust-lang/book/pull/783/files from all the HTML files for the book for all the versions...... which is arguably "more correct", but also more work.

That is modifying the code we released, which is against immutable releases, which I am pretty against.

have you confirmed that adding the store.js file to a book built with mdbook 0.22 fixes the styling?

I am doing so right now.

@aidanhs aidanhs added the C-bug Category: This is a bug. label Sep 19, 2017
@steveklabnik
Copy link
Member Author

Okay, so this is strange.

rustup doc --book shows... no rendering problems, even though store.js is 404. However, if I mdbook serve, so that it's not on file://, it does reproduce. So, this does mean that local stable is working, which is nice.

However.... if I do use mdbook serve, and add store.js, it resolves to a 200, but still has the style issues. O_O.

More investigation is apparently needed.

@steveklabnik
Copy link
Member Author

Every version ever published with mdbook now looks broken https://doc.rust-lang.org/1.17.0/book/

I remember checking this before release; there's no way that the book has been broken for three releases and nobody has noticed.

@aturon
Copy link
Member

aturon commented Sep 19, 2017

cc @rust-lang/infra

@steveklabnik
Copy link
Member Author

steveklabnik commented Sep 19, 2017 •
edited
Loading

(@ashleygwilliams and I are debugging this right now)

@steveklabnik
Copy link
Member Author

So, @ashleygwilliams figured it out:

<body class=""light"">

Yes, there are extra quotes in there. However, they're not in view-source, only in the final output. This makes me suspect JavaScript.

But it's still extremely suspicious that not one person noticed this, not sure why this is happening.

@ashleygwilliams
Copy link
Member

if you change the theme, the error disappears. so that's a quick fix to offer people.

taking a look at the old javascript to see if i can discern what's up.

@steveklabnik
Copy link
Member Author

Okay! We think we've got it.

mdbook stores the current theme in localstorage. This did change in newer/older mdbook, but it looks like this, basically:

    var theme = localStorage.getItem('theme');
    if (theme === null) { theme = 'light'; }

However, if you visit the nightly docs, and run this in your JS console:

>> localStorage.getItem("theme")
"\"light\""

See the quotes? If you go back to stable docs, and delete the theme from localstorage, and then refresh, it works, and

>> localStorage.getItem("theme")
"light"

So: TL;DR: this is an mdBook bug. It only manifests if you read the nightly docs, then go back and look at the stable docs.

So, the solution is, fix the bug in mdBook, update, and backport to beta.

@steveklabnik steveklabnik mentioned this issue Sep 19, 2017
Closed
@steveklabnik steveklabnik removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 19, 2017
@steveklabnik
Copy link
Member Author

So, the solution is, fix the bug in mdBook, update, and backport to beta.

Correction: only nightly needs to be fixed; it's what's poisoning localstorage. No need to update the store.js file either, so un-tagging @rust-lang/infra .

Whew! This bug was nasty.

@carols10cents
Copy link
Member

😱🙀😱🙀😱🙀😱🙀

@steveklabnik steveklabnik mentioned this issue Sep 21, 2017
Closed
@frewsxcv
Copy link
Member

Was this resolved upstream with https://github.com/azerupi/mdBook/pull/449 ? If so, is the next step here just an mdbook bump?

@steveklabnik
Copy link
Member Author

Yes, we need an mdbook release and then bump the version here.

@carols10cents carols10cents mentioned this issue Sep 26, 2017
Merged
@Michael-F-Bryan Michael-F-Bryan mentioned this issue Sep 30, 2017
Open
steveklabnik added a commit to steveklabnik/rust that referenced this issue Oct 2, 2017
@steveklabnik
update mdbook
1bbf41d 
Fixes rust-lang#44704
@steveklabnik steveklabnik mentioned this issue Oct 2, 2017
Merged
bors added a commit that referenced this issue Oct 2, 2017
@bors
Auto merge of #44977 - steveklabnik:update-mdbook, r=alexcrichton
bdcb7fb 
update mdbook

Fixes #44704
@ExpHP
Copy link
Contributor

ExpHP commented Oct 10, 2017

To my understanding this fix has prevented new cases of corrupted local storage from being created, but does not help the existing cases. From what I gather, localStorage never expires, so users who do not regularly clear their data may remain affected for arbitrarily long.

@steveklabnik
Copy link
Member Author

steveklabnik commented Oct 10, 2017 •
edited
Loading

Yes. To be clear, you don't even need to know anything about localStorage to fix this issue; you can click this little paintbrush icon to get a theme dropdown:

theme

and choose any theme, then pick your original theme. Boom, fixed.

@carols10cents carols10cents mentioned this issue Oct 26, 2017
Closed
@XX
Copy link

XX commented Oct 31, 2017

@steveklabnik I confirm. But I chose the same theme (Light) and the styles were restored immediately.

@steveklabnik
Copy link
Member Author

Yeah, any theme choice should do it. Thanks for the clarification :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@steveklabnik @carols10cents @XX @frewsxcv @aturon @aidanhs @ashleygwilliams @ExpHP