Skip to content
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

Decouple themes from core functionality #3636

Open
pawamoy opened this issue Apr 14, 2024 · 17 comments
Open

Decouple themes from core functionality #3636

pawamoy opened this issue Apr 14, 2024 · 17 comments
Labels

Comments

@pawamoy
Copy link
Sponsor Contributor

pawamoy commented Apr 14, 2024

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.

@tomchristie
Copy link
Member

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

@tomchristie
Copy link
Member

Also related...

(And possibly others?)

@squidfunk
Copy link
Sponsor Contributor

squidfunk commented Apr 18, 2024

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

Options

Option 1 – Move business logic and readthedocs theme out

  • mkdocs (depends on mkdocs-core and includes the default theme)
  • mkdocs-core (everything that is currently in mkdocs minus the themes)
  • mkdocs-readthedocs (depends on mkdocs-core)

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

  • mkdocs (everything that is currently in mkdocs minus the themes)
  • mkdocs-classic (depends on mkdocs and includes the default theme)
  • mkdocs-readthedocs (depends on mkdocs)

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.

@squidfunk squidfunk changed the title Move readthedocs theme out of core Decouple themes from core functionality Apr 18, 2024
@pawamoy
Copy link
Sponsor Contributor Author

pawamoy commented Apr 18, 2024

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

@waylan
Copy link
Member

waylan commented Apr 18, 2024

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
Copy link
Sponsor Contributor

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
Copy link
Member

This approach?...

  • Create packages for both of them. (Repo here for RTD if anyone wants to get started.)
  • Switch to them as dependancies. (And move issues across to those packages as repos.)
  • Then in a position to easily unbundle / switch defaults. (Tho we don't need to decide on that yet, and can decouple that decision from the packaging legwork that comes first.)

@squidfunk
Copy link
Sponsor Contributor

@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 #3636 (comment), 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
Copy link
Sponsor Contributor

A crossover of Option 2 and what @tomchristie suggested in #3636 (comment)

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

  • mkdocs (has mkdocs-classic and mkdocs-readthedocs as dependencies)
  • mkdocs-core (everything that is currently in mkdocs minus the themes)
  • mkdocs-classic (has mkdocs-core as a dependency)
  • mkdocs-readthedocs (has mkdocs-core as a dependency)

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:

  • mkdocs depends on mkdocs-classic, mkdocs-readthedocs
  • mkdocs-classic depends on mkdocs-core
  • mkdocs-readthedocs depends on mkdocs-core

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
Copy link
Member

tomchristie commented Apr 19, 2024

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
Copy link
Sponsor Contributor Author

pawamoy commented Apr 19, 2024

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
Copy link
Sponsor Contributor

@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
Copy link
Sponsor Contributor Author

pawamoy commented Apr 19, 2024

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
Copy link
Member

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

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

@squidfunk
Copy link
Sponsor Contributor

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 4ea78da. I'm not sure whether this behavior is currently defined.

@pawamoy
Copy link
Sponsor Contributor Author

pawamoy commented Apr 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants