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

Search in 0.17.0 is incompatible with third-party themes #1316

Closed
squidfunk opened this Issue Oct 20, 2017 · 24 comments

Comments

Projects
None yet
3 participants
@squidfunk

squidfunk commented Oct 20, 2017

Firstly, congratulations on version 0.17. Really nice to see MkDocs is evolving. I read about the new plugin system and tried to upgrade to MkDocs 0.17. I also added this to my mkdocs.yml:

plugins:
  - search

However, the index doesn't seem to be generated and MkDocs is complaining about a search.html not being present. Material defines the search.html as a partial, as it's part of the main theme.

$ mkdocs serve
INFO    -  Building documentation... 
INFO    -  Cleaning site directory 
INFO    -  Template skipped: 'search.html'. Not found in template directories. 
[I 171020 09:30:09 server:283] Serving on http://127.0.0.1:8000
[I 171020 09:30:09 handlers:60] Start watching changes
[I 171020 09:30:09 handlers:62] Start detecting changes
[I 171020 09:30:28 handlers:133] Browser Connected: http://localhost:8000/
[W 171020 09:30:29 web:2063] 404 GET /mkdocs/search_index.json (127.0.0.1) 1.79ms

Any directions?

@waylan

This comment has been minimized.

Member

waylan commented Oct 20, 2017

I'm not sure why its not work for you as I haven't taken a look at your source, but here are some notes about the changes we made to the builtin themes which may help.

Of course, search works for both of the included themes, each of which works differently. The mkdocs theme uses a modal box rather than a search.html template, so it generates the "no search.html" warning (as it always has). However, the readthedocs theme provides a search.html template, which works fine (with no warnings).

In the readthedocs theme, the only change we needed to make to the search.html template was to remove the script tags which included the search libs as the plugin handles that for us separately (by adding entries to the extra_javascript config setting). Note that the plugin includes all of the search related files (libs and index file) in a search/ sub-dir, which is a different location than before. The old script tags won't find things in their new location and the search.js is now loaded differently (its not passed as a perimeter to require.js but is a standalone require-wrapped script). Of course, if you leave it to the plugin to handle, it just works (the theme should not need a search/ sub-dir; the plugin alters the theme's Jinja environment by adding its search/ subdir). I should note that the search.html template still needs to be at the root of the theme; we didn't change that.

Additionally, we needed to make a couple changes to base.html of the readthedocs theme: (1) define the base_url JavaScript var (which the search script needs) and (2) wrap the "searchbox" displayed on every page in a {% if 'search' in config['plugins'] %} check (see also this change were we now call it search). The second change allows a theme to support either having the plugin enabled or disabled. You should be able to support both older and newer versions of MkDocs in third party themes by using {% if 'plugins' in config %} (something we didn't need to do in the builtin themes as they only get used with the version they ship with).

To be clear, I haven't really looked at any third party themes with consideration to how they might interact with plugins. If there is anything we should be doing different to make things easier, let us know. That's part of the reason we released the Plugin API in 0.17. We still have time to make changes before 1.0.

@waylan

This comment has been minimized.

Member

waylan commented Oct 20, 2017

One additional note: I only did the minimum necessary to get search working as a plugin. The original plan was to completely refactor the search plugin to address all of the issues listed in the search refactor project. But that didn't happen yet. If it had, search would be a completely new thing and the changes would probably be more easily justified. However, now that we have everything controlled by the plugin, the future refactor should have less impact on themes. In other words, if you make the changes to work with this, you won't need to make many (if any) adjustments in the future when the search refactor happens.

@waylan

This comment has been minimized.

Member

waylan commented Oct 20, 2017

... and now that I have actually read your output, I see this:

[W 171020 09:30:29 web:2063] 404 GET /mkdocs/search_index.json (127.0.0.1) 1.79ms

As I pointed out above, the search index would now be at /search/search_index.json so, of course, that returned a 404.

@squidfunk

This comment has been minimized.

squidfunk commented Oct 22, 2017

Thanks for your extensive answer. The warning about the missing search.html is actually new I think - haven't seen it before. The rest is pretty self explanatory. Is the mkdocs.yml extending mkdocs_theme.yml? Material provides a search integration and I would like to enable the search plugin by default. Is that possible? Or do I have to tell users to enable it? Otherwise a lot of sites will deactivate search when they upgrade to 0.17.

@squidfunk

This comment has been minimized.

squidfunk commented Oct 22, 2017

The search plugin seems to automatically include two files:

<script src="./search/require.js"></script>
<script src="./search/search.js"></script>

How can I disable this? Material provides an own search logic that doesn't need the code.

@waylan

This comment has been minimized.

Member

waylan commented Oct 23, 2017

The warning about the missing `search.html is actually new I think - haven't seen it before.

Its not new. Perhaps it was only displayed with the --verbose flag previously. At least it was less obvious before. Perhaps it still should be. After all, it is more relevant to theme authors than the general user. More on this below.

Is the mkdocs.yml extending mkdocs_theme.yml?

mkdocs_theme.yml sets defaults for a theme. The theme setting of the mkdocs.yml allows the user to override those defaults. Its a more sophisticated replacement for extra. The documentation is here

I would like to enable the search plugin by default.

It is. As the release notes explain:

If no plugins setting is defined in the config, then the search plugin will be included by default.

And as explained in the configuration docs:

If the plugins config setting is defined in the mkdocs.yml config file, then any defaults (such as search) are ignored and you need to explicitly re-enable the defaults if you would like to continue using them... To completely disable all plugins, including any defaults, set the plugins setting to an empty list.

The search plugin seems to automatically include two files... How can I disable this?

You have two options: (1) provide your own replacement plugin or (2) provide your own replacement files in the theme. I'm assuming you would want to use option 2, although option 1 would be as simple as a subclass which re-implemented step 1 below.

Perhaps it would help to explain how the search plugin works. The code is here and its not very complicated. But I'll break it down into the three basic steps:

  1. It alters the config (immediately after config validation is completed) in a few ways (this is probably most relevant to you):

    1. It adds its own templates dir to the Jinja env. This is using normal Jinja precedence and it has the lowest precedence. A theme with files of the same path/name will override.

    2. It adds search.html to the static_templates setting. This is a new setting which is part of the new theme customization (defined in mkdocs_theme.yml). Any files included in this setting are built as standalone pages (not from Markdown). A theme could use this to add a site index, a search page, or any other static content that doesn't fit into the Markdown generated content. The template simply needs to be available in the Jinja env at the path provided.

      By the way, this is where the "missing search.html" warning is coming from. A template file doesn't exist that was expressly requested in a setting. Previously, search.html was rendered with dedicated code for that one file only. Now it is one of any number of possible user and/or theme defined templates. It doesn't make sense for the code to single that one out to alter the warning. I suppose the plugin could re-implement template rendering for that file as a one-off. But it would complicate things and I'd rather not.

    3. The files search/require.js and search/search.js are added to the extra_javascript setting. That is why you are getting these files even though they are not included in your template. As stated above, if you provide your own replacement files with these paths, the theme provided files will take precedence over the plugin provided files. As an aside, it never occurred to me that this would be undesirable for a third party theme. Not sure of a better solution though.

  2. A search index is created and built up as the build steps though all of the pages.

  3. The search/search_index.json file is written to the site_dir.

@squidfunk

This comment has been minimized.

squidfunk commented Oct 23, 2017

Thanks for your long answer and excuse me not reading the docs properly, mainly due to a lack of time on my side. However, I have some remaining questions:

You have two options: (1) provide your own replacement plugin or (2) provide your own replacement files in the theme. I'm assuming you would want to use option 2, although option 1 would be as simple as a subclass which re-implemented step 1 below.

Option 1 increases potential errors on the user side. I would have to instruct the user to always install my custom search plugin with the theme. That's not practical. Can the theme itself provide a plugin or do I have to register a whole new project in pip? If the later is the case, this would really, really complicate everything for Material and degrade user experience. Besides the original MkDocs theme and the ReadTheDocs theme, Material seems to be one of the most used, so this may impact a lot of users.

Option 2 is not really an option, because Material implements the search logic itself. Material only depends on the search_index.json and no other files, so I would really like to exclude them. For this reason it's not convenient to provide the search in those files and to use require.js for loading stuff. Everything is bundled in Material, so no need for require.js. Is the search_index.json provided by MkDocs or by the search plugin? If the first is the case, I should be fine with just disabling it.

I have to pin Material to 0.16.3 until this is resolved, because now it breaks one way or the other way. However, I have no idea how to tackle this in a practical way which is transparent to the user. The last part is really important because a lot of build environments will break with option 1 and option 2 always shows errors of missing files in the console.

@waylan

This comment has been minimized.

Member

waylan commented Oct 23, 2017

Can the theme itself provide a plugin

There isn't any reason why this couldn't work (sort of). Both themes and plugins are simply Python modules with an entry_point to point to them. You can have more than one entry_point in a package (the MkDocs package includes two themes and one plugin right now). Just include each in a separate subdir and point the entry_points appropriately.

The one disadvantage is that the user still needs to list both in the config manually. You could have a plugin which changes the theme setting, but there is no way for a theme to change the plugin setting. And it would be weird to have users set the theme via a plugin (also not backward compatible).

Option 2 is not really an option, because Material implements the search logic itself. Material only depends on the search_index.json and no other files, so I would really like to exclude them.

As a reminder, the existing search functionality is expected to undergo a complete refactor prior to the next release (primarily the JavaScript end of things). For example, I expect that require.js will no longer be used. If a third-party theme is providing its own search logic, why can't that logic be contained in a file at search/search.js? As I explained previously, the theme gets precedence over the plugin, so the theme's file would get used rather than the plugin's. Prior to our removal of require.js, I suppose you could provide an empty file at search/require.js (its only referenced from search.js). Yes, that feels like a hack, but it will only be for the one version.

Another solution just occurred to me. Plugins can have their own config options. The current plugin has none because it is simply replicating the existing behavior (which had no configuration options). However, we could add a config option to the plugin which only created the search_index.json file and left all other functionality to themes to implement. Perhaps something like:

plugins:
    - search:
        index_only: true

Would that work for you? I realize that would still break for existing users until they updated their configs, but it would give you what you actually want going forward. And the "fix" for existing users would be to add that one setting.

As an aside, I had not realized that you (or any third-party theme, for that matter) were implementing your own search logic. Knowing that would likely have informed some of the decisions that were made. When discussing the current solutions for both the Plugin API and the Theme Customization feature, I remember begin surprised that no theme devs chimed in. PRs were posted and sat for days waiting for feedback before being merged. The silence was interpreted as no objections. Perhaps I was wrong in making that assumption. That's a significant factor in releasing 0.17 rather than going straight to 1.0. We're not locked in yet.

@waylan waylan changed the title from Search seems broken in 0.17 to Search in 0.17 is incompatible with third-party themes Oct 23, 2017

@waylan

This comment has been minimized.

Member

waylan commented Oct 23, 2017

we could add a config option to the plugin which only created the search_index.json file and left all other functionality to themes to implement.

Perhaps even better, We could make the default dependent on a theme option. That way, the default behavior would depend on the theme. For example, Material's mkdocs_theme.yml file might include:

search_index_only: true

Whereas the mkdocs theme would have:

search_index_only: false

We could even define a setting for search modal vs. a dedicated search page, which would control whether search.html was added to the static_templates setting. That way, themes which don't provide a search.html would avoid the missing file warning.

@squidfunk

This comment has been minimized.

squidfunk commented Oct 23, 2017

Short answer (long answer later): The search-index only solution would be perfect. When this is implemented, I can fix all incompatibilities and release Material 2.0 which requires MkDocs 0.17.

Also the search-partial vs search-template fix would be awesome!

EDIT: thinking about it again, it would be better to have a solution that a theme defines plugin dependencies and default configuration. I know, there's no option for that, but then the nice setup instructions of "just add theme: material" are getting over-complicated... hmm...

@waylan

This comment has been minimized.

Member

waylan commented Oct 23, 2017

it would be better to have a solution that a theme defines plugin dependencies and default configuration.

We have the default config (for theme specific stuff), but I'm not sure how the plugin dependencies would work. Would the theme define its own plugins setting in mkdocs_theme.yml? Would that need to be merged with the plugins setting in the users mkdocs.yml? I suppose its not impossible, but there is no clear path for resolving conflicts between the two. And the documentation would be less than clear. I can see why this could be attractive to a theme dev, but I doubt its necessary. In fact, the built-in themes work with and without the search plugin. Yes, it requires a little more work on the theme side, but we do occasionally have users request a way to completely disable search. I don't think it makes sense to have a theme "require" any specific plugin. Enabling or disabling a plugin should simply turn some features of a theme on or off.

The one exception is that a Plugin could define its own "theme specific" config options. That way, the theme can inform a plugin which way it expects the plugin to work. To demonstrate, consider the search.html situation...

There are currently two ways to have a search.html page generated from a template: (1) enable the search plugin or (2) list search.html in the theme.static_templates config option. If you check the readthedocs theme (which uses a search.html template), search.html is not listed in its static_templates setting in mkdocs_theme.yml because the user could disable the search plugin (in which case we don't want the file included in static_templates). Therefore, adding search.html to static_templates is left to the plugin to handle. I realize trying to balance these sorts of dependency conflicts can get confusing and I went back and forth on the above issue before settling on current behavior. But that is still less than ideal as the mkdocs theme (for example) dies not use a search.html template and currently raises that warning (which could cause users to incorrectly think they did something wrong).

My proposed solution is to add another theme setting which is specific to the search plugin. When true, it basically says: only when the search plugin is enabled, then add search.html to static_templates. When false it would simply ensure the search plugin never adds search.html to static_templates. Of course, if the search plugin is disabled, it would be ignored regardless what value it was given ,The documentation for such a setting would be contained in the search plugin documentation. A theme dev would not need to inform their users about it unless the theme supported two options (search modal or dedicated search page). In that case, the theme would just use the existing setting and not need to define its own additional setting.

@waylan

This comment has been minimized.

Member

waylan commented Oct 23, 2017

I just pushed a proposed fix to #1322. See the documentation for an explanation of the proposed changes. Actually, I noticed while working on this that the "search" documentation was outdated and wrong in some cases. Its clear I didn't give enough thought to that side of things when I was working on the Plugin API. I hadn't made a single change to that part of the documentation, In retrospect, I recall thinking as some point i would come back to that after working out some other issues with the Plugin API. Apparently I never did.

@squidfunk

This comment has been minimized.

squidfunk commented Oct 24, 2017

So from what I see in the docs and your notes I could now provide a mkdocs_theme.yml with the following setup:

plugins:
  - search:
      search_index_only: true
      include_search_page: false

Is that correct? Also, what are the default options for those attributes? And for the user, configuration would be done with just defining theme: name: THEMENAME and not list the search plugin explicitly, as this is done by the theme config. If this is the case, then this should solve the problem introduced with 0.17 :-)

@squidfunk

This comment has been minimized.

squidfunk commented Oct 24, 2017

Reading the Python code in your PR, I assume I wouldn't have to provide anything at all. Even better, but I would set it explicitly in Material, so it doesn't break if at some point in the future the default values are changed.

@waylan

This comment has been minimized.

Member

waylan commented Oct 24, 2017

You almost have it. You cannot (and I expect never will be able to) define a plugin in mkdocs_theme.yml. The new settings are "theme" settings, not "plugin" settings. In the themes' mkdocs_theme.yml file you would do:

search_index_only: true
include_search_page: false

The user would still need to have the plugin enabled in their MkDocs config. Of course, it is enable by default, but the user could disable it and any theme MUST account for that.

Also as a warning, a user COULD override the defaults you set in her mkdocs.yml config file. I would suggest not documenting it unless you want to offer both options, but you should be aware that the user could do:

theme:
    name: material
    search_index_only: false
    include_search_page: true

Technically there are no defaults. If they are not defined by the theme (in its mkdocs_theme.yml file), then they are not defined at all. I would look at it this way: the defaults are what are set by the theme (in its mkdocs_theme.yml file). However, there is a default behavior if the setting is not defined at all, which is documented for each setting.

@waylan

This comment has been minimized.

Member

waylan commented Oct 24, 2017

By the way, the new settings are not "plugin" settings (you cannot define them under the plugin in mkdocs.yml) as they are completely reliant on a theme's support for them. I don't expect that these would be settings that an end user would (or should) be altering in most cases. They are solely for theme devs. Of course, they can still be set under theme in mkdocs.yml as there is no mechanism to prohibit that. However, for those using a custom_dir for their theme, they may need to set the values in their mkdocs.yml file (and the custom_dir does not use a mkdocs_theme.yml file). So, the values should be editable by the end user for that reason. Still, they are theme related , so they are "theme" settings and cannot be defined under the "plugin".

@squidfunk

This comment has been minimized.

squidfunk commented Oct 24, 2017

Okay, got it. That would solve the problems with 0.17! Thank you very much for your time, patienc and work. When can we expect 0.17.1? Are there any other issues that need to be solved before?

@waylan

This comment has been minimized.

Member

waylan commented Oct 24, 2017

I expect 0.17.1 will be out before next week. There are a couple other possible issues, but they seem very minor.

@waylan

This comment has been minimized.

Member

waylan commented Oct 27, 2017

Fixed in #1322.

@waylan waylan closed this Oct 27, 2017

@squidfunk

This comment has been minimized.

squidfunk commented Oct 29, 2017

Any update on 0.17.1?

@waylan

This comment has been minimized.

Member

waylan commented Oct 29, 2017

Unfortunately, I ran out of time on Friday. I'm guessing Monday or Tuesday.

@waylan

This comment has been minimized.

Member

waylan commented Oct 30, 2017

0.17.1 has been released.

@breezedu

This comment has been minimized.

breezedu commented Nov 2, 2017

Well, in my case, there's no "mkdocs" dir under site/, so I copied and pasted the mkdocs folder from my prious site, now the search function is back.
Plus, I am using mkdocs v-0.17.1, Python 3.6. Win10.

@waylan

This comment has been minimized.

Member

waylan commented Nov 2, 2017

@breezedu it sounds like you are using a theme which has not been updated for 0.17.

@waylan waylan changed the title from Search in 0.17 is incompatible with third-party themes to Search in 0.17.0 is incompatible with third-party themes Nov 2, 2017

dsagal added a commit to gristlabs/mkdocs-windmill that referenced this issue Jun 26, 2018

Search page needs to be included to allow showing 'all results'
Also include search settings into instructions for using custom_dir,
since theme's mkdocs_theme.yml is ignored in that case (see
mkdocs/mkdocs#1316 (comment))

dsagal added a commit to gristlabs/mkdocs-windmill that referenced this issue Jun 26, 2018

Include search.html page and update documentation and example.
The search page needs to be included to allow showing 'all results'.

In documentation, what used to be 'theme_dir' is now 'theme.custom_dir'.

Also include search settings into instructions for using custom_dir,
since theme's mkdocs_theme.yml is ignored in that case (see
mkdocs/mkdocs#1316 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment