Only load searchindex when needed #2553
Conversation
|
If you're going to mess with the search engine, can you also write some gui tests that exercise it? |
|
Very good point. |
GuillaumeGomez
force-pushed
the
load-on-need
branch
from
8ab48db to
8b5125d
Compare
|
Added the GUI test. :) |
tests/gui/sidebar-nojs.goml
Outdated
| @@ -2,9 +2,6 @@ | |||
| // an iframe (because of JS disabled). | |||
| // Regression test for <https://github.com/rust-lang/mdBook/issues/2528>. | |||
|
|
|||
| // We disable the requests checks because `searchindex.json` will always fail | |||
| // locally. | |||
| fail-on-request-error: false | |||
GuillaumeGomez
force-pushed
the
load-on-need
branch
from
e3244b2 to
16cbfd6
Compare
|
When the JS is being retrieved, there is now a spinner and the input is not disabled anymore:
|
GuillaumeGomez
force-pushed
the
load-on-need
branch
2 times, most recently
from
38b4ea7 to
c96b7e4
Compare
|
Rebased and fixed merge conflict. |
|
It looks like you reverted the loading throbber, and now it disables the search input while it's loading again. 16cbfd6 added it, but the pull request doesn't have it any more. |
|
Arg indeed. Rebase went wrong I guess. Great catch, thanks! |
|
Everything seems good here! |
GuillaumeGomez
force-pushed
the
load-on-need
branch
from
a002ecb to
27d7465
Compare
|
Rebased. |
GuillaumeGomez
force-pushed
the
load-on-need
branch
from
27d7465 to
afd2be1
Compare
|
Re-rebased and also ran |
GuillaumeGomez
force-pushed
the
load-on-need
branch
from
afd2be1 to
5557b3f
Compare
|
Rebased. If you could take a look @ehuss. If you need help to understand what this PR is doing, don't hesitate to ask! |
|
Can you say more about the reasoning behind this change? For me, it comes off as a worse experience, so we are somehow not seeing the same thing. For example, after opening a book and looking at it, I may want to search for something. Today this loads instantly (since it is eagerly loaded), but with this change I may need to wait 10+ seconds for anything to happen. Is this trying to save bandwidth? Or is it some performance issue? I'm not completely against this kind of change, but the description here doesn't explain the motivation or acknowledge the potential downsides. |
I'm very surprised. What book do you have this 10+ seconds wait? The whole point of this PR is to actually speed up the page load and reduce the (page) size by default until you actually need the search. This is becomes more and more useful as the book size (and its search index) grows. This is how we allow rustdoc to always load extremely quickly, whatever the size of the crate (and of its search index). |
|
The RFCs book has a 22MB index. I suppose 10s might be a bit of an exaggeration for many people, as I was just doing some throttling tests, though I think we should be sensitive to people without fast internet. Can you help me understand how this speeds up page load? My understanding is that the index is loaded async in the background. My expectation is that since it is loading in the background, it shouldn't have any measurable effect on initial render time. What little page-load profiling I've done doesn't show a difference with this PR. |
|
It's mostly for people with limited internet access (not in bandwidth but in data) that will get their life improved with this PR: it only loads extra content on demand.
I wasn't clear, my bad. The load time should normally be close to not impacted (except I forgot to convert the JS to |
|
Ah I found a good analogy: with the RFC book, instead of loading the 22MB search index on all pages whether you need or not, you will only load it when you want to perform a search. |
This PR makes it so that the
searchindex.jsfile is only loaded when the user arrives on a page with?search=in the URL or if the user opens the search. It means that the book size will be only the text content until the user actually needs to run a search.cc @notriddle