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

refactor(blog): theme-common shouldn't depend on blog content plugins #10313

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jul 18, 2024

Breaking change?

Internal APIs have been moved, this is not considered a breaking change requiring a new major version, although swizzle users might need to update docs/blog theme-common imports in their code:

-import {useDoc} from '@docusaurus/theme-common/internal';
+import {useDoc} from '@docusaurus/plugin-content-docs/client';
-import {useBlogPost} from '@docusaurus/theme-common/internal';
+import {useBlogPost} from '@docusaurus/plugin-content-blog/client';

Motivation

We have had a coupling problem for a while, that led to issues such as cyclic dependencies when trying to implement certain features in plugin-specific client exports.

All plugin-specific browser code should now be in @docusaurus/<contentPlugin>/client and not @docusaurus/theme-common that should remain plugin-agnostic.

For now, I only refactored the blog plugin code. The docs plugin will follow but it's more difficult.

Edit: docs follow-up PR #10316

Test Plan

Unit tests + CI + preview

Test links

https://deploy-preview-10313--docusaurus-2.netlify.app/

@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Jul 18, 2024
@slorber slorber requested a review from Josh-Cena as a code owner July 18, 2024 17:35
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 18, 2024
Copy link

socket-security bot commented Jul 18, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@argos-ci/playwright@1.9.3 Transitive: environment, filesystem +5 172 kB neoziro
npm/@netlify/functions@1.6.0 None +1 20.6 kB netlify-bot
npm/@playwright/test@1.45.2 None 0 25.4 kB yurys

View full report↗︎

Copy link

github-actions bot commented Jul 18, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO Report
/ 🟠 72 🟢 98 🟢 100 🟢 100 Report
/docs/installation 🟠 58 🟢 97 🟢 100 🟢 100 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟠 86 Report
/blog 🟠 70 🟢 96 🟢 100 🟠 86 Report
/blog/preparing-your-site-for-docusaurus-v3 🔴 47 🟢 92 🟢 100 🟢 100 Report
/blog/tags/release 🟠 68 🟢 96 🟢 100 🟠 86 Report
/blog/tags 🟠 75 🟢 100 🟢 100 🟠 86 Report

Copy link

netlify bot commented Jul 18, 2024

[V2]

Name Link
🔨 Latest commit 38b8ae9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/669a6b7b2e6d9c0008245cee
😎 Deploy Preview https://deploy-preview-10313--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Jul 18, 2024

Size Change: 0 B

Total Size: 1.85 MB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/codeTranslations.json 2 B
website/.docusaurus/docusaurus.config.mjs 27.4 kB
website/.docusaurus/globalData.json 123 kB
website/.docusaurus/i18n.json 930 B
website/.docusaurus/registry.js 306 kB
website/.docusaurus/routes.js 203 kB
website/.docusaurus/routesChunkNames.json 131 kB
website/.docusaurus/site-metadata.json 2.17 kB
website/build/assets/css/styles.********.css 113 kB
website/build/assets/js/main.********.js 909 kB
website/build/index.html 38.1 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator

Last time we brought this up, I think you were suggesting that theme-common is really an internal detail for deduplication between different built-in themes—so the design choice has shifted?

@slorber
Copy link
Collaborator Author

slorber commented Jul 19, 2024

Last time we brought this up, I think you were suggesting that theme-common is really an internal detail for deduplication between different built-in themes—so the design choice has shifted?

At the time we introduced theme-common (nov 2020 #3775) we couldn't use yet /client exports due to node version and tooling.

Now we can, and started introducing some /client exports in a few packages.

But the thing is, we have to make a decision: either theme-common depends on all content-plugin, or the opposite, but we can't have a cyclic dependency.

Having docs/blog code spread in both theme-common and docs/blog client exports is problematic because for some feature I wanted to implement previously, I wanted to import things from both dependencies, and couldn't because it introduces a cycle.

I now think it's better to move plugin-specific code to their respective plugin client export. It will also be easier to write public API documentation for those APIs.

If you think about it, most of this code is code that supports the plugin on the client side. It is not really related to theming, but rather style-agnostic code that simply manipulates the data/props/context that the plugin made available.

Theme-common has reusable utils, but if you think about it, it remains a bit opinionated. If I were to create a new blog plugin from scratch that does not look like the official docusaurus theme, and if I don't plan to use theme-common for it, all those functions probably still make sense to use and are quite unopinionated/theme-agnostic

export {
  BlogPostProvider,
  type BlogPostContextValue,
  useBlogPost,
  useBlogMetadata,
} from './contexts';

export {
  useBlogListPageStructuredData,
  useBlogPostStructuredData,
} from './structuredDataUtils';

export {
  BlogSidebarItemList,
  groupBlogSidebarItemsByYear,
  useVisibleBlogSidebarItems,
} from './sidebarUtils';

(one could argue useVisibleBlogSidebarItems is a bit opinionated, assuming a blog necessarily has a sidebar for example, and items can be hidden, but those opinions are already in the content plugin anyway)

The way I see it:

  • content-plugin/client: code that read props/context, query them, transform them in convenient ways. No theming/styling. No more opinions than what the content plugin has.
  • theme-common: code with reusable utils and code that support the creation of our core theme(s) classic/tailwind. No styling, but can be a bit opinionated regarding the UX we implement.

Hope this makes sense

Note that we could eventually split later theme-common in 2: the unopinionated frontend/React utils VS the theme/UX opinionated parts. That does not seem necessary to do this right now though.

@slorber slorber changed the title refactor: theme-common shouldn't depend on blog content plugins refactor(blog): theme-common shouldn't depend on blog content plugins Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants