search

mkdocs / mkdocs

Project documentation with Markdown.
https://www.mkdocs.org
BSD 2-Clause "Simplified" License
18.49k stars 2.36k forks source link

Decouple themes from core functionality #3636

Open pawamoy opened 1 month ago

pawamoy commented 1 month ago

During the backlog grooming call we decided to move the ReadTheDocs theme out of core, in a separate repository, and as a separate installable Python package.

pawamoy commented 1 month ago

Related issues:

tomchristie commented 1 month ago

I've created a repo here... https://github.com/mkdocs/theme-readthedocs and given the @mkdocs/core team access to it.

tomchristie commented 1 month ago

Also related...

(And possibly others?)

squidfunk commented 1 month ago

I've thought about this for a while now and currently see two options:

Options

Option 1 – Move business logic and readthedocs theme out

In this setting, mkdocs would remain the installable entrypoint for users. Everything that is not theme related however is moved to mkdocs-core, so plugins can use that as a dependency to leave out the theme stuff that is not necessary. The default theme would be located in mkdocs. Thus, from an end user as well as extension and plugin author perspective nothing would change. Plugin authors could then move their plugins to mkdocs-core without breakage.

Additionally, users could just install mkdocs-readthedocs, which will automatically pull in mkdocs-core – we've been following this approach almost since the beginning of mkdocs-material, so that users do not explicitly have to install mkdocs. Additionally, it allows themes to be explicit about the version of mkdocs that they support.

Option 2 – Move mkdocs and readthedocs themes out

In this setting, we would move out both themes, and make mkdocs-classic the installable entrypoint. Since MkDocs always needs a theme, it would mean that the user would be explicit about the theme that is installed, again the same as with mkdocs-material. No changes to plugins would be necessary, but it would mean a breaking change for all users, since mkdocs will come without a theme now.

Recommendation

I'm leaning towards Option 1. Many libraries move their business logic out into a *-core package, so it's a common pattern that MkDocs could follow in order to "trim the fat" and make things more modular. Also, this option would include no breaking changes and would not even mandate a deprecation path, because everything still works. Plugin authors moving to mkdocs-core would be favorable, but in the end could be considered cosmetics.

Both options would allow to decouple changes to MkDocs' business logic from its themes. Currently, a theme change on the mkdocs or readthedocs theme always mandates a new release of the mkdocs package, which might even be a breaking theme change that does not touch business logic and would require a major release, albeit having not impact on core functionality or plugins. Thus, one way or the other, IMHO, this makes a lot of sense.

Feedback is very much appreciated! Maybe there's some option that we're currently not seeing, so we should let this sit here for a while and then gravitate towards a solution we can all agree on.

Edit: I've allowed myself to generalize the title, as only moving readthedocs out will only solve part of the problem. I hope that this aligns with the goals and vision of the other maintainers. If not, feel free to change it back.

pawamoy commented 1 month ago

Thanks for the detailed suggestions @squidfunk. I think I like option 1 too.

waylan commented 1 month ago

I like option 1 as well.

I will note that the existing mkdocs-bootstrap and mkdocs-bootswatch themes depend on and inherit from the mkdocs theme. With option 1 they will continue to work without modification, whereas option 2 would be a breaking change. I don't know, but it is possible that the same situation exists for other third-party themes.

I will add one counterpoint to this. Moving the readthedocs theme out will be a breaking change for some users. Originally, the readthedocs service provided their own theme and we simply provided a copy for testing, etc. However, with later changes to both MkDocs and to the service, users could configure the service to use either the theme provided by the service or the theme provided by MkDocs. Generally those who chose the later option were advanced users who wanted/needed the additional flexibility. With this change, those who have chosen the later option will have their sites broken by this change and will need to go in and change their site config to point to a different Python package. I have not used this myself and the details are fuzzy to me, but I do recall receiving some support requests about this in the past and it was always a challenge to work out which option the user chose and whether it was something we could help with or not. Usually, those who were actually using the built-in readthedocs theme always knew that they were because they had made a conscious choice to do so. They will know this change applies to them if/when they become aware of it. But it may be confusing to the less advanced users who (probably) won't need to do anything.

To be clear, I'm not saying I think we shouldn't move forward with this. However, we will want to give some consideration to those users who use MkDocs with the readthedocs service.

squidfunk commented 1 month ago

I will add one counterpoint to this. Moving the readthedocs theme out will be a breaking change for some users.

Excellent observation that slipped my attention.

When following option 1, we could have a transition period where we automatically install mkdocs-readthedocs alongside mkdocs (make it a dependency) and issue a deprecation notice with a warning that the readthedocs theme now needs to be installed separately. I'm not sure how this could be done (during installation?) but maybe this could somehow be pulled off.

tomchristie commented 1 month ago

This approach?...

squidfunk commented 1 month ago

@tomchristie I'm not sure I follow. As I understand from what you write, you're proposing to follow what I described as Option 2: move both themes out. Have you considered the breaking changes that this might incur which I described in https://github.com/mkdocs/mkdocs/issues/3636#issuecomment-2062860413, or do you have something in mind that mitigates this problem which I'm not currently seeing? 😅 Because, what's the point where you'd be removing the dependencies? IMHO, we'd only be deferring this problem, so if we decide to follow this approach, we should have a plan in place, including a deprecation period.

We just had a case where it would be super helpful if we would've decoupled the themes from core, as removing jQuery from the mkdocs theme, we could do a major release on the mkdocs theme, but keep the version of core:

squidfunk commented 1 month ago

A crossover of Option 2 and what @tomchristie suggested in https://github.com/mkdocs/mkdocs/issues/3636#issuecomment-2064427861

Option 3 – Move everything out and convert mkdocs to a shell

IMHO, if we make mkdocs a shell, we would absolutely need to move the business logic out into a mkdocs-core package, so themes could be explicit about what version of MkDocs they support. IMHO, this is a critical requirement. mkdocs-material had several major releases in the past because MkDocs had released breaking changes. This is just around the corner again with version 1.6.0, which I regard as problematic for the number of changes that it brings. This is also the reason why we limited our version range to MkDocs 1.5.

We would need to be careful not to introduce cyclic dependencies, so the dependency graph could be:

As far as I see it, third-party themes would not need to change their dependency on mkdocs if they specified one, but could and should change to mkdocs-core. As such, mkdocs-material would also switch to mkdocs-core.

tomchristie commented 1 month ago

I'm not sure I follow. As I understand from what you write, you're proposing to follow what I described as Option 2.

I'll clarify, I was aiming at Option 0... *

"Yeah, I'm not sure if we should go to the park or the beach. Maybe let's get dressed and have some breakfast, and we can have a think about it while we're doing that."

* careful readers will notice some differences between my suggestion and the simile used. hopefully it conveys the gist, tho.

pawamoy commented 1 month ago

When following option 1, we could have a transition period where we automatically install mkdocs-readthedocs alongside mkdocs (make it a dependency) and issue a deprecation notice with a warning that the readthedocs theme now needs to be installed separately. I'm not sure how this could be done (during installation?) but maybe this could somehow be pulled off.

Unless we play with the presence/absence of specific packages after installation, another way to show deprecation warnings is to keep vendoring the theme during the transition period, and emit warnings when the vendored files are used (can be done from Python code loading the theme, or from the Jinja templates themselves if we inject a logger into the Jinja context).

squidfunk commented 1 month ago

@pawamoy excellent idea – vendoring could be a viable deprecation path ✌️

@tomchristie Okay, let me try to interpret your beautiful prose 😅 It sounds like you're suggesting to move out the readthedocs theme and add it as a dependency in the mkdocs package, so it is installed alongside, right? What about the mkdocs theme? Will that be distributed with the mkdocs package in the mid-term? Should it be factored out as well?

I tried to outline some options all with up- and downsides, in order to get some more eyes on this. That already paid off, as we found some further implications that would need to be considered. Only moving out readthedocs will not allow us to specify which version of mkdocs it is compatible with. If that does not matter, we can do that, but I'd say it could be important, especially to keep things stable and deliberately decide into what versions to opt in.

IMHO, for the restructuring to be successful, how we cut should align with the vision of MkDocs that we and especially you see. In my opionion, we shouldn't just start for the sake of doing something. That's at least how I see it.

pawamoy commented 1 month ago

Not sure what @tomchristie meant, but I suppose we can start copying (not moving) the ReadTheDocs theme into the new repository. This would allow us to start playing with deprecation messages (as mentioned in my comment above).

tomchristie commented 1 month ago

we can start copying (not moving) the ReadTheDocs theme into the new repository.

Exacyl, yes... https://github.com/mkdocs/theme-readthedocs/

squidfunk commented 1 month ago

If we'd opt for vendoring, we'd end up with two themes with the same name, so we'd need to make sure that precedence works in our favor, similar to what I did for plugins in https://github.com/mkdocs/mkdocs/commit/4ea78da2e93e1165c2600df5f5a66704ff8bc197. I'm not sure whether this behavior is currently defined.

pawamoy commented 1 month ago

Good point. Maybe the external package could use entry point metadata, while the vendored files would not. If we don't find the specified theme through regular logic, and the theme is "readthedocs", we load it from vendored files (and emit a warning).