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(docs,theme): split DocItem comp, useDoc hook #7644

Merged
merged 6 commits into from Jun 22, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 17, 2022

Breaking change

  • @theme/DocItem component has been split into much smaller subcomponents
  • @theme/DocItemFooter has been moved to @theme/DocItem/Footer and does not receive props anymore
  • You can access previous props data with the new useDoc() hook

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

This is a follow-up of moving in the direction to:

  • using context instead of props => more flexible
  • split large component into smaller ones

We already did that in multiple places as well (doc sidebar etc...)

This PR does:

  • put doc route data in the context
  • access data through a new useDoc() hook, that will likely become part of our public API
  • define that public API more explicitly (ie we don't have to stick to the shape in which data is injected into comps => implementation detail)
  • split components into much smaller subcomponents, easier to swizzle
  • make it easy to change the doc item layout without swizzling a large file

We'll likely mark some of these components as safe to swizzle later?

Test Plan

Preview works as before

Test links

Deploy preview: https://deploy-preview-7644--docusaurus-2.netlify.app/docs

Related issues/PRs

Somehow related: #7637 (useDoc() will make this easy)

- put doc route data in context
- access data through useDoc() hook
- split components into much smaller subcomponents
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 17, 2022
@netlify
Copy link

netlify bot commented Jun 17, 2022

[V2]

Name Link
🔨 Latest commit 154a022
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62b3022ca0822c0009067493
😎 Deploy Preview https://deploy-preview-7644--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 settings.

@github-actions
Copy link

github-actions bot commented Jun 17, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🔴 28 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 84 🟢 100 🟢 100 🟢 100 🟢 90 Report

Copy link
Collaborator Author

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for a review before merge

/**
* Decide if the toc should be rendered, on mobile or desktop viewports
*/
function useDocTOC() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we'll want to extract this hook later?

Not 100% satisfied with it atm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this—seems like over-abstraction that hides its semantics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you suggest?

I'd like the layout component to be easy to edit visually, without the TOC technical details noise.

Maybe it's fine to keep this weird hook, but just keep it there so that users can understand it more easily? 🤷‍♂️

I don't like it either but I don't have much better to suggest without polluting the component

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when I'm swizzling, I'd personally like to gain some understanding of what the hook is doing. This is especially important for non-TS users. This hook is congregating too many unrelated things. It's fine if it's in the same file and easily viewable, but extracting it to theme-common makes it much harder to work with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree 👍

let's ship this as is and see how to improve later

packages/docusaurus-theme-common/src/contexts/doc.tsx Outdated Show resolved Hide resolved
* but we may want to change that in the future.
* This layer is a good opportunity to decouple storage from consuming API.
*/
function useContextValue(content: PropDocContent): DocContextValue {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion: storing metadata/frontmatter in the mdx file is an implementation detail. Consuming the data with Component.metadata doesn't feel very natural to me so I'd rather not stick to it 🤷‍♂️

In practice, we could create separate bundles/props for these metadata. And it may have advantages to do so in the future if for example we want to provide some kind of "data query layer" (I mean, for example we want to display some docs/blog metadata in the homepage, without having to load all the mdx content in that page)

I'm not sure how things will turn out, but I'd prefer to decouple the metadata from the MDX component when consuming it. I'd like to do something similar on the blog as well.

Copy link
Collaborator

@Josh-Cena Josh-Cena Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree—the metadata should be a separate chunk. This means we can easily provide the same metadata as both props and route context. Seems too complex in the short term, though.

@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jun 17, 2022
@github-actions
Copy link

github-actions bot commented Jun 17, 2022

Size Change: +18 B (0%)

Total Size: 801 kB

ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 52.6 kB 0 B
website/build/assets/css/styles.********.css 106 kB 0 B
website/build/assets/js/main.********.js 603 kB +18 B (0%)
website/build/index.html 38.9 kB 0 B

compressed-size-action

@Josh-Cena Josh-Cena changed the title refactor(docs,theme): refactor DocItem: split in small comps + useDoc() refactor(docs,theme): split DocItem comp, useDoc hook Jun 22, 2022
function useContextValue(content: PropDocContent): DocContextValue {
return useMemo(
() => ({
MDXComponent: content,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to expose this as context? Looks like TMI to me 😄

Copy link
Collaborator Author

@slorber slorber Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's part of the static data being available in the subtree, so not sure how it could be used, but why not?

Eventually we may move all this subtree context data to be made available at the plugin level? Using our secret useRouteContext? #6885
(I mean useDoc() ➡️ useRouteContext().doc, no need for DocContext)

You'd prefer to forward a children prop?

I think both solutions are fine 🤷‍♂️ it's unlikely someone would want to render that component in a weird place like the Footer, so forwarding children could also work

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this is useless in all cases to be exposed as a context, since only the main container would care about it and it doesn't really encourage any code reuse. (e.g. no one would be forwarding this to some subcomponent as props.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you, I'm fine if you revert it to children if you think it's better, both look fine to me

Comment on lines +31 to +38
return useMemo(
() => ({
metadata: content.metadata,
frontMatter: content.frontMatter,
assets: content.assets,
contentTitle: content.contentTitle,
toc: content.toc,
}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we intentionally cherry-picking the properties from content on both type and value, instead of using a spread?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes because I'd like to avoid mdx loader export implementation details to automatically leak into an API that we'll likely make public later

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Just confirming

@lanegoolsby
Copy link
Contributor

@slorber is useDoc exported? I tried to import it in my component but get an error:

import React, { useState } from 'react';
import TOC from '@theme-original/TOC';
import { useDoc } from '@docusaurus/theme-common/src/contexts/doc';

export default function TOCWrapper(props: any) {
  const docMeta = useDoc();
  const [tags, setTags] = useState<any[]>([]);

  function asdf() {
    if (docMeta.frontMatter.tags && docMeta.frontMatter.tags.length > 0) {
      setTags(docMeta.frontMatter.tags);
    }
  }

  return (
    <>
      <TOC {...props} />
      Hello world
    </>
  );
}
Module not found: Error: Package path ./src/contexts/doc is not exported from package /my/repo/path/node_modules/@docusaurus/theme-common (see exports field in /my/repo/path/node_modules/@docusaurus/theme-common/package.json)

@TasoOneAsia
Copy link

@lanegoolsby Was looking into utilizing this hook as well, and from preliminary investigation seems like the useDoc hook is only being exported under the internal entry point for the time being. Should be able to import it by referencing that sub-path instead, i.e.

import { useDoc } from '@docusaurus/theme-common/internal'

@lanegoolsby
Copy link
Contributor

Typescript and/or VSCode doesn't like that

image

@Josh-Cena
Copy link
Collaborator

@lanegoolsby You need to compile with moduleResolution: NodeNext.

@lanegoolsby
Copy link
Contributor

Can this hook be used in a custom plugin? I can use it in a swizzled TOC, but I get this error when I try to use the same code in a custom plugin.

image

I am using a /docs path - /docs/tutorial-basics/create-a-document

I tried both with and without a <DocProvider>, both throw the same error.

import React, { useState } from 'react';
import { useDoc } from '@docusaurus/theme-common/internal';
import TOC from '@theme-init/TOC';
import { DocProvider } from '@docusaurus/theme-common/internal';

export default function (props: any) {
  const docMeta = useDoc();

  return (
    <DocProvider content={props.content}>
      <TOC {...props} />
      Hello world <code>{JSON.stringify(docMeta)}</code>
    </DocProvider>
  );
}

@slorber
Copy link
Collaborator Author

slorber commented Aug 8, 2022

useDoc() is only supposed to be used on a doc page. Other pages (blog, pages or your custom plugin routes) do not have the same data available so you'd have to use another hook, eventually creating your own.

I'm not sure to understand at all what you are trying to do here 😅 please show a repro.

Note technically you could reuse DocProvider + useDoc (a React context is just a way to provide a value to a whole subtree), but these comp/hook are created with TS typings for the data made available through the docs plugin, so if you are looking to pass a very different set of data you'd rather create your own React context instead.

Note: you can't call the useXyz() hook in the same component that uses the provider <ProvideXyz>. What you do above is always wrong. You can only use the hook on components in the provider subtree. You can generally solve this problem by extracting what's inside the provider to a new custom React component, and using the hook there.

@lanegoolsby
Copy link
Contributor

lanegoolsby commented Aug 8, 2022

I'm trying to setup a custom plugin that injects a custom React component immediately below the Table of Contents component, for example:

image

Where the Hello World is, I'd like to have a component that will read all the tags for a doc page, query an internal API with said tags via HTTP/REST, and render the results in a grid.

@slorber
Copy link
Collaborator Author

slorber commented Aug 9, 2022

I'm trying to setup a custom plugin that injects a custom React component immediately below the Table of Contents component

Note the TOC component is more a "design system component" and it's not really meant to be coupled to a specific usage (docs, blog, pages). I mean it's not supposed to use useDoc() or useBlog(). Now we don't prevent it but I think you'd rather avoid creating such coupling.

If you only want to enhance the Doc TOC (and not the page/blog TOC), you should rather swizzle
@theme/DocItem/TOC/Desktop (instead of @theme/TOC) => this component can safely use useDoc()

Also don't forget to think about the mobile experience where the TOC is a collapsible at the top of the document

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: breaking change Existing sites may not build successfully in the new version. Description contains more details.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants