mkdocs-material: Search returning entire page in search box [mkdocs-material insiders]

Contribution guidelines

I’ve found a bug and checked that …

  • … the problem doesn’t occur with the mkdocs or readthedocs themes
  • … the problem persists when all overrides are removed, i.e. custom_dir, extra_javascript and extra_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:

image

Actual behaviour

The insiders version of mkdocs-material makes the search result return all of the page in its entirety. image

Steps to reproduce

  1. Add tags to pages using mkdocs-material insiders
  2. Commit to GitHub repository
  3. 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}}      
  1. Test out search
  2. 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)

Most upvoted comments

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 still have to comment that I don’t like that what you’re developing totally bypasses upstream. Your expertise could’ve been so useful, but instead we’re gathered here to solve an issue that exists just because this is not upstreamed. I mean, how can you brag that it’s so much better when you never offered any of that to upstream and it’s paywalled. Anyway, I’ll also admit it’s not as simple as I make it sound, there are arguments to your decision, even good technical arguments, but yeah…

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:

    • MkDocs’ search plugin – as noted, I did a complete rewrite of the indexing strategy
    • Lunr.js’ tokenizer – also, a complete rewrite, since we’re now indexing HTML, not plain text
    • Material for MkDocs search implementation – a rewrite of the indexing strategy and how search results are resolved

    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 and readthedocs 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!

I’ve seen it mentioned that if people don’t like the search they can write their own plugin, is there really no way to have a search plugin identified the same way in templates unless they override the MkDocs search?

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 MkDocs localsearch plugin, which post-processes the search index built by the search plugin and must come after it in mkdocs.yml. Luckily, as it relies on the search plugin, the template must not be adjusted. However, a plugin that replaces the search plugin would need to be accounted for in the template, unless my proposal would be merged.

I always assumed that if you wrote your own search, it would work as a drop-in replacement, if that isn’t how it works, then it seems a reasonable request to MkDocs to make it so.

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

  1. Insiders provides its own search plugin implementation. It exposes the plugin via entry_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.

  2. 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 via importlib_metadata. On my system, importlib_metadata.entry_points() returns:

    [
      EntryPoint(name='search', value='mkdocs.contrib.search:SearchPlugin', group='mkdocs.plugins'),
      EntryPoint(name='search', value='material.plugins.search.plugin:SearchPlugin', group='mkdocs.plugins'),
      EntryPoint(name='social', value='material.plugins.social.plugin:SocialPlugin', group='mkdocs.plugins'),
      EntryPoint(name='tags', value='material.plugins.tags.plugin:TagsPlugin', group='mkdocs.plugins')
    ]
    

    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:

    [
      EntryPoint(name='search', value='material.plugins.search.plugin:SearchPlugin', group='mkdocs.plugins'),
      EntryPoint(name='social', value='material.plugins.social.plugin:SocialPlugin', group='mkdocs.plugins'),
      EntryPoint(name='tags', value='material.plugins.tags.plugin:TagsPlugin', group='mkdocs.plugins'),
      EntryPoint(name='search', value='mkdocs.contrib.search:SearchPlugin', group='mkdocs.plugins')
    ]
    

    This means that the built-in search plugin takes precedence. I don’t yet understand why, it seems unpredictable.

  3. 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 the header 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:

git clone --depth 1 https://${GH_TOKEN}@github.com/squidfunk/mkdocs-material-insiders.git
pip install -e mkdocs-material-insiders

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:

def get_plugins():
    """ Return a dict of all installed Plugins as {name: EntryPoint}. """

    plugins = importlib_metadata.entry_points(group='mkdocs.plugins')

    pluginmap = {}
    for plugin in plugins:
        if plugin.name in pluginmap and "mkdocs.contrib" in plugin.value:
            continue

        pluginmap[plugin.name] = plugin

    return pluginmap

@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.