-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Remove direct search index reference from searcher #2742
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
Remove direct search index reference from searcher #2742
Conversation
21505fa
to
a3daea5
Compare
Because `{{resource}}` references don't affect the hash[^1], we need to avoid referencing dynamic content from within static content. Otherwise, you get a cached searcher.js referencing a searchindex that no longer exists. [^1]: if we made it affect the hash, we'd have to do full dependency tracking, and we'd no longer be able to support circular refs
a3daea5
to
e96c608
Compare
Nice, thanks! |
{{#if search_js}} | ||
window.path_to_searchindex_js = "{{ resource "searchindex.js" }}"; | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to be careful with changes like this, since it looks like it will break anyone who has overridden index.hbs. Is there any way to avoid that breakage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an official policy for breaking changes in *.hbs
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we try to avoid any breaking changes (except across major releases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted but that is a big brake on any changes in this file or even for this format. Is it an official feature and mentioned as such or just something we provide as a best effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can avoid breaking change by re-hash the searcher URL, though I understand it takes much higher effort than this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are referring to index.hbs
being customizable, yes it is documented here: https://rust-lang.github.io/mdBook/format/theme/index-hbs.html#indexhbs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would adjusting the searcher.js
script to use the old way, if index.hbs hasn't been updated, be an acceptable solution?
diff --git a/src/front-end/searcher/searcher.js b/src/front-end/searcher/searcher.js
index e92c1c854a..17af071aec 100644
--- a/src/front-end/searcher/searcher.js
+++ b/src/front-end/searcher/searcher.js
@@ -434,7 +434,7 @@ window.search = window.search || {};
function showSearch(yes) {
if (yes) {
- loadSearchScript(window.path_to_searchindex_js, 'search-index');
+ loadSearchScript(window.path_to_searchindex_js || (path_to_root + '{{ resource "searchindex.js" }}'), 'search-index');
search_wrap.classList.remove('hidden');
searchicon.setAttribute('aria-expanded', 'true');
} else {
This means updating mdbook won't fix the bug on its own, if you haven't also updated your theme, but it still solves the problem for @tuyen-at-work (and we can remove this hack next time a breaking release goes out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR was created, I used a hacky way, that is append the searchindex hash into searcher's URL:
Live example can be found here.
I used Bash script to achieve it, not very nice but does solve the cache issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That diff looks like it should work to me, thanks!
Fixes #2741
Because
{{resource}}
references don't affect the hash1, we need to avoid referencing dynamic content from within static content. Otherwise, you get a cached searcher.js referencing a searchindex that no longer exists.Footnotes
if we made it affect the hash, we'd have to do full dependency
tracking, and we'd no longer be able to support circular refs ↩