mkdocs-material: Search returning entire page in search box [mkdocs-material insiders]
Contribution guidelines
- I’ve read the contribution guidelines and wholeheartedly agree
I’ve found a bug and checked that …
- … the problem doesn’t occur with the
mkdocs
orreadthedocs
themes - … the problem persists when all overrides are removed, i.e.
custom_dir
,extra_javascript
andextra_css
- … the documentation does not mention anything about my problem
- … there are no open or closed issues that are related to my problem
Description
Using mkdocs-material insiders
search function seems to be returning the entire pages content alongside the paragraph symbol in the search results. This only occurs using deployment on GitHub pages, not locally.
Expected behaviour
I expect the search function to work the same on deployment as it does locally and not display the entire content of the page in the search tool, as follows:
Actual behaviour
The insiders version of mkdocs-material makes the search result return all of the page in its entirety.
Steps to reproduce
- Add
tags
to pages usingmkdocs-material insiders
- Commit to GitHub repository
- Use GitHub actions to deploy:
jobs:
deploy:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
with:
python-version: 3.x
env:
- run: pip install git+https://${ALBERT_KEY}@github.com/squidfunk/mkdocs-material-insiders.git
env:
ALBERT_KEY: ${{secrets.ALBERT_KEY}}
- Test out search
- Search bug appears.
Package versions
- Python:
Python 3.9.5
- MkDocs:
mkdocs, version 1.2.2
- Material:
7.2.8+insiders.3.0.1
Configuration
site_name: LA Metro Digital Services Team Docs
repo_url: https://github.com/LACMTA/digital-services-team-docs/
theme:
palette:
- scheme: default
primary: black
toggle:
icon: material/toggle-switch-off-outline
name: Switch to dark mode
- scheme: slate
primary: black
accent: white
toggle:
icon: material/toggle-switch
name: Switch to light mode
font:
text: Open Sans
favicon: assets/media/favicon.png
logo: assets/media/favicon-reverse.png
name: material
features:
- navigation.instant
- navigation.tabs
- content.code.annotate
- search.highlight
- toc.integrate
- navigation.indexes
- navigation.top
- search.suggest
icon:
repo: fontawesome/brands/github
markdown_extensions:
- pymdownx.superfences
- pymdownx.magiclink
- smarty
- meta
- toc:
permalink: True
toc_depth: 2
- sane_lists
- admonition
- footnotes
- attr_list
edit_uri: edit/dev/docs/
plugins:
- git-revision-date
- autorefs
- table-reader
- macros
- tags
- search:
lang: en
System information
- Operating system: Windows 10
- Browser: Firefox 92.0
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 19 (14 by maintainers)
I’ve opened a pull request in https://github.com/mkdocs/mkdocs/pull/2591 with the necessary changes. I’ve also updated the OP to list the workaround to new users experiencing this issue.
I just released 7.3.3-insiders-3.1.3, bumping the minimum MkDocs version to 1.2.3 which contains the fix. Updating to this version resolves this issue, which means it can be closed. Thanks to all of you for helping to work towards a solution!
Thanks for the detailed response. Indeed these are solid arguments.
@oprypin thanks for your review of https://github.com/mkdocs/mkdocs/pull/2591!
I understand that it looks like it makes sense to upstream it. Let me explain why I believe that at this point in time, it doesn’t:
Breaking change – all themes (not only the built-in ones, but all third-party themes) would need to adapt to this change if this change would be upstreamed, i.e., when the built-in search plugin would be replaced by the new implementation. This is why Insiders also had a major release. I believe it makes more sense to first release it as a sister plugin of Material for MkDocs, fix any issues we discover on the way, and then at a later point check if this change can be applied to all themes.
Note that I needed to patch the following components to make it work:
The fact that we’re now indexing HTML and not text implied the Lunr.js tokenizer rewrite, which a theme must correctly anticipate and handle. From what I’ve learned on the way, switching over to indexing HTML instead of text is not a boolean decision – it needs to be carefully accounted for. Yes, we could also try to upstream the tokenizer to Lunr.js, but same as for MkDocs – too much potential breakage, and probably too opinionated.
Actually, I’ve wanted to write another blog article how indexing and highlighting now works from a theme perspective, as I think it’s quite interesting from a technical standpoint. I’ll do that when I find some time.
Faster iterations – Historically, MkDocs has had a rather slow and conservative release cycle, which I think is good in general, as it more or less guarantees that the core is stable. Unfortunately, this doesn’t foster innovation to happen. Luckily, the plugin system is so well designed that innovation can happen inside plugins, which allows for faster iteration. I decided to make this plugin part of Insiders because it took significant effort to pull this off, and I believe that it is better to first test a change of this magnitude in a constrained environment before it is made generally available.
Furthermore, if this would be upstreamed or released as a standalone plugin, I, personally, would also have to support compatibility with other third-party themes which I currently can’t commit to. Material for MkDocs had a custom search implementation since its first public release because IMHO, the UX of the
mkdocs
andreadthedocs
themes is not that great. The new implementation would need to be factored out and abstracted into something that can be re-integrated into Material for MkDocs and other third-party themes. I don’t have the time to do it, and given the demographics of this project, I don’t see how there are many front-end developers who will be willing to invest their spare time to pull this off. In my experience, front-end developers rather use Docusaurus, Gatsby, or Vuepress.Now, regarding the “bragging”: It was not my intention to brag. If that’s what you get from the blog article, I’d be curious to learn what exactly you read as bragging, because I tried to make it as scientific and data-driven as possible. I also explained the rationale behind the changes, diving into the details on how search is currently working, and how I believe it could be improved. My intention was to show the huge potential that lies in rethinking this topic. If you wish to give constructive criticism, ping me at martin.donath@squidfunk.com (or on Gitter), as I don’t want to hijack this thread for something offtopic.
Also, I get it: you don’t like what I’m doing with Insiders. We’ve had a previous discussion on this regarding your section index plugin. But please, understand: I’m not forcing anybody to use Insiders. It’s entirely optional. Even without Insiders, this theme is probably the most feature-rich in the MkDocs ecosystem and there’s nothing that forces users into Insiders. Everything is either an improved drop-in replacement or behind a feature flag.
Additionally, I’m doing the best I can to triage issues quickly, answer discussions, etc. I’m doing many bug fixes (check the commit history and last releases) on this repository and add new features (hello new content tabs!) which I don’t put into Insiders, behind a – as you say – “paywall”. Please take a look at the list of features that were only possible to develop with the financial support of my sponsors, because otherwise, I wouldn’t have the time. Also, you’re using at least some previously exclusive features on your documentation projects, so you’re actually benefiting from it.
Again, if you feel there’s something to be discussed, please ping me (email above). I’m happy to discuss any concerns.
@facelessuser thanks for your take on this!
The
search
plugin is just a plugin, so the only way to identify it in templates is by its name. I know of no other reliable way. For example, there is the MkDocslocalsearch
plugin, which post-processes the search index built by thesearch
plugin and must come after it inmkdocs.yml
. Luckily, as it relies on thesearch
plugin, the template must not be adjusted. However, a plugin that replaces thesearch
plugin would need to be accounted for in the template, unless my proposal would be merged.I will open a PR for this, which can be used as a further basis for discussion. I really hope for this to make its way into
master
, because it would make migration a breeze 😊Okay, so I can finally reproduce this issue and think I’ve found the cause of the problem.
Insight and observations
Insiders provides its own
search
plugin implementation. It exposes the plugin viaentry_points
with the same name, because it is a full replacement for MkDocs built-in search plugin. In fact, the built-in search plugin causes the behavior that can be seen (full page content in first result), which means that since Insiders 3.0 it is essentially not supported anymore, as it is superseded by the custom implementation.Exporting a plugin with the same name works in most, but not call cases. I haven’t found out why it sometimes works and why it doesn’t. The problem manifests in the
get_plugins
function in MkDocs core, which loads the plugin entry points viaimportlib_metadata
. On my system,importlib_metadata.entry_points()
returns:Since the built-in plugin is loaded first, it is overwritten by the custom search plugin. However, I’ve managed to reproduce the described error in GitHub Actions, where
importlib_metadata.entry_points()
returns:This means that the built-in search plugin takes precedence. I don’t yet understand why, it seems unpredictable.
Why renaming the search plugin is not a good option. So, we could just rename the search plugin or prefix it, right? Theoretically, yes. However, this would probably lead to more confusion, because the search plugin is enabled by default, so users now always have to explicitly list the new search plugin. It would also mean that some templates might break, because we’re explicitly checking for the
search
plugin in some places. Several users override theheader
partial, so we would have to carry out a major release, with the potential for a lot of issues rolling in, since search is so widely used.Workaround
As a temporary workaround, this installation method seems to work reliably:
Mitigations
The best idea I could think of would be to patch this function in MkDocs to effectively allow for overriding the search plugin. Personally, I believe this is a sound case. After all, the MkDocs team always advertised that the built-in search plugin is only meant for demonstration purposes, and they’ve always encouraged users to provide alternate implementations. Here we have an alternate implementation that is superior in many ways.
The necessary patch would be:
@oprypin, @facelessuser, @ultrabug – looping you in for a constructive discussion on this topic. If there’s any other possibility without patching core I’m happy to discuss. Nonetheless, I’d expect the proposed patch to not break anything, and to be sound in terms of community-contributed plugins.