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

RFC: Improve theming DX (swizzle) #6114

Open
slorber opened this issue Dec 16, 2021 · 5 comments
Open

RFC: Improve theming DX (swizzle) #6114

slorber opened this issue Dec 16, 2021 · 5 comments
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee proposal This issue is a proposal, usually non-trivial change
Milestone

Comments

@slorber
Copy link
Collaborator

slorber commented Dec 16, 2021

As part of #6113, we want to improve the swizzle DX

The docusaurus swizzle command works fine but still has some major DX problems.

These are various proposals that we can work on before 2.0.

Not all these features are mandatory for v2, but for me the minimum is:

  • Refactor code
  • Refactor doc
  • docusaurus theme wrap
  • docusaurus theme override
  • Refactor getSwizzleComponentList

Code

The swizzle CLI command code is not the best part of our codebase, it is quite hard to read, not very well tested and easy to read

Proposal: as part of the current RFC, we should rewrite everything in a better way, designing a whole new DX at the same time. It should be easier than modifying the current code.

Naming

swizzle is a term that is a bit confusing. At least to me, and already saw other feedback

As a non-native English speaker, I've discovered this term in Docusaurus for the first time and had to translate it to understand what it means. Today, I'm still not sure that this translation is even correct 😅 (I translate it as melt or re-arrange 🤷‍♂️ )

Proposal: rename to docusaurus theme override or docusaurus theme copy

Documentation

The swizzle doc is part of the advanced guides, interleaved with other docs such as how to author a theme. It does not feel right to me.

We should have a dedicated place that group all the possible ways to customize a Docusaurus site in a single place: using CSS file, environment variables, swizzle...

Proposal: probably move that doc to https://docusaurus.io/docs/styling-layout#styling-approaches

wrap

docusaurus swizzle is something that you should rather use as a last resort, as you copy internal source code in your own project.

A better pattern is to use wrap a component so that you can add some logic before/after the original component.

import OriginalFooter from "@theme-original/Footer";
import React from "react";

export default function Footer(props) {
  return (
    <>
      <div>Before footer</div>
      <OriginalFooter {...props} />
    </>
  );
}

This reduces the API surface maintained by the user, ensuring retro-compatibility as long as the public API of the component (props) does not change.

Proposal: docusaurus theme wrap command. Eventually maintain

getSwizzleComponentList

This theme lifecycle is not flexible enough to convey what is our public theme API surface.

For example:

  • some components might be safe to wrap but not to override (some layout items...)
  • some components might be safe to wrap and override (icons...)

If swizzle is renamed to docusaurus theme override, this lifecycle name should also be updated.

It may be useful to provide more than 2 levels of safety? 🤔 (safe, warn, unsafe?)

When adding a new theme component, a test should ensure that we never forget to declare the level of safety for wrap/override, to prevent having an unclear public API surface.

Proposal: this needs a complete redesign. We can safely remove the existing getSwizzleComponentList, it is optional and probably not used by many community plugins anyway.

diff

When overriding theme components, we should have a way to easily inspect how our local implementation has changed compared to the upstream theme implementation.

Proposal: docusaurus theme diff to see a diff of all overridden files, with options for all at once or focusing on a single file

Problem: how to clearly identify comps that override from comps that wrap?

upgrade

Can we provide tooling so that on Docusaurus upgrades, users that override our theme are able to patch their local files?

Already mentioned in #5734 (comment)

It could work like this:

  • on docusaurus theme override, store the current version (let's say 2.3.1) in the copied file (user is expected to keep this comment)
  • on docusaurus theme upgrade: use current version (2.4.8) create a theme file diff between 2.4.8 & 2.3.1 => apply this diff to the copied file + upgrade comment to 2.4.8

Other proposed solution here: #5734

Proposal: docusaurus theme upgrade

Problem: it might be complex to build, probably not a top priority atm 🤷‍♂️

Upgrade Helper website

Users should be able to easily inspect the changes we did in our themes.

React-Native has such a helper website, allowing to see changes across 2 versions: https://react-native-community.github.io/upgrade-helper/?from=0.62.0&to=0.67.0-rc.0

When publishing changelogs and major release blog posts, we could allow our community to browse changes easily with a simple link. Users could understand faster if the upgrade requires theme changes.

Proposal: build a helper website to browse a diff online, with some useful filters (theme/plugin...)

Problem: duplicate of the diff command? Which one is the most important to build first, with the best ROI?

debug

Having a CLI command to easily inspect what are the unsafe overrides of a theme that are currently in use might be useful (particularly when we might change what is safe/unsafe over time).

This cli could do multiple things:

  • print a diff of unsafe overridden files
  • show various recommendations like replacing an override by wrap...
  • provide more useful features later

Proposal: docusaurus theme debug, docusaurus theme inspect, docusaurus theme doctor...

Problem: naming? builds on top of other features: later?


Let me know what you think, this is just a draft for now and all these proposals are very incomplete in term of design.

@slorber slorber added proposal This issue is a proposal, usually non-trivial change status: needs triage This issue has not been triaged by maintainers labels Dec 16, 2021
@slorber slorber changed the title RFC: Improve swizzle DX [2.0] [2.0] RFC: Improve swizzle DX Dec 16, 2021
@slorber slorber added this to the 2.0.0 milestone Dec 16, 2021
@slorber slorber removed the status: needs triage This issue has not been triaged by maintainers label Dec 16, 2021
@slorber slorber changed the title [2.0] RFC: Improve swizzle DX [2.0] RFC: improve theming DX (swizzle Dec 16, 2021
@slorber slorber changed the title [2.0] RFC: improve theming DX (swizzle [2.0] RFC: Improve theming DX (swizzle Dec 16, 2021
@slorber slorber changed the title [2.0] RFC: Improve theming DX (swizzle [2.0] RFC: Improve theming DX (swizzle) Dec 16, 2021
@Josh-Cena
Copy link
Collaborator

My two cents.

Naming

My first time seeing this term as well, and your understanding is generally correct: it's to "stir" or "mix". In the documentation there's a brief explanation: "In iOS, method swizzling is the process of changing the implementation of an existing selector (method). In the context of a website, component swizzling means providing an alternative component that takes precedence over the component provided by the theme."

I like this term very much. It's playful, unique, and not coming out of thin air either. If you look at its Objective-C origin, it's exactly the same as what we are doing.

swizzle --eject and swizzle --wrap would be enough to differentiate them. I like --eject better than --copy since it also alludes to the danger: CRA treats eject as the ultimate customization.

@yangshun As the original implementer, what do you see here?

Documentation

Swizzling should always be considered advanced, since nowhere in our "Guides" do we actually ask you to write JavaScript to implement a core feature! We should certainly refactor our advanced guides into more Recipes / Key concepts style.

Wrap

Very easy to implement, pretty easy to understand, should definitely be prioritized

Diff

Agree. For now I have a header comment to remind myself of what I have changed. We should try to have a git merge kind of DX, like what yarn patch offers.

Problem: how to clearly identify comps that override from comps that wrap?

Just check if it imports @theme-orginal. Nothing else would have this.

getSwizzleComponentList

I don't dislike this and would like it kept. We can change the API to exporting a list of objects, each with the component's name, safety, even file path, etc.

(Also, this isn't a lifecycle, this is a metadata of the theme itself.)

upgrade

Complicated, but certainly worth the effort. It's also going to help users migrate some config options that we discourage / deprecate.

Upgrade Helper website

Duplicate of the diff command, indeed, and I think diff should come first since it's more limited in scope.

debug

Even later. We can have a Docusaurus linter.


In summary, my alternative proposal is:

docusaurus swizzle --eject # Outputs a complete file
docusaurus swizzle --wrap # Outputs a wrapper component
docusaurus swizzle --patch # Opens a temporary file for you to edit, and then saves it as a diff file. At runtime, we merge the diff file with the actual theme component, and throw if they don't reconcile.

docusaurus upgrade theme # Try to merge the new component with the existing one, lets you solve a git conflict if they don't

@slorber
Copy link
Collaborator Author

slorber commented Jan 6, 2022

That seems reasonable thanks

For v2 we should probably just focus on --eject and --wrap.

I'd like to change a bit the behavior of getSwizzleComponentList (maybe rename it) because some components might be safe to wrap, but not to eject, and we should probably be able to assign more config (and even a description) to each component

--patch and upgrade may be more complicated to achieve in the short term, let's figure out

@Josh-Cena
Copy link
Collaborator

Yes, that sounds like my plan as well! This issue would be good to be worked on. For specific details about --wrap let's discuss in #5380

@Josh-Cena
Copy link
Collaborator

Wild thought: is it possible to not specify themeName when running swizzle? Since Docusaurus knows which component is actually used at runtime, can we do the same in swizzling and automatically resolve the right component to be swizzled?

For example, currently, if the user uses live-codeblock, but chooses to swizzle theme-classic/CodeBlock (maybe because she wasn't aware of the internal inheritance), the @theme/CodeBlock would point to the swizzled one and the live function will be entirely lost.

@slorber
Copy link
Collaborator Author

slorber commented Jan 12, 2022

Wild thought: is it possible to not specify themeName when running swizzle? Since Docusaurus knows which component is actually used at runtime, can we do the same in swizzling and automatically resolve the right component to be swizzled?

Yes that makes sense to me to swizzle the "final component" (ie live-codeblock) in priority, as long as there's an option to eventually swizzle the original theme component (ie we should still be able to swizzle both as one might extend the other)

Another little detail is to support the shorthand names like classic and probably display classic first in the list

@slorber slorber changed the title [2.0] RFC: Improve theming DX (swizzle) RFC: Improve theming DX (swizzle) Aug 17, 2023
@slorber slorber modified the milestones: 2.0.0, Upcoming Aug 17, 2023
@slorber slorber added the apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee proposal This issue is a proposal, usually non-trivial change
Projects
None yet
Development

No branches or pull requests

2 participants