refactor(theme-common): split package into public/internal API entrypoints #7660

slorber · 2022-06-22T17:08:26Z

Breaking change

Many @docusaurus/theme-common APIs have been moved to @docusaurus/theme-common/internal: if your swizzled components stop working, try updating the imports
APIs have not been renamed/refactored, so just changing the import might work
You might need to split the import in 2 parts (public/internal)

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

We want to make it more explicit which @docusaurus/theme-common APIs are public, and which are internal.

We'll maintain semver retrocompatibility only on public APIs.

Work in progress, details to be discussed.

IMHO there is 3 kind of APIs:

@docusaurus/theme-common public APIs with guaranteed retrocompatibility
- documented: recommended APIs to use
- undocumented: we ensure retro compatibility but don't encourage direct usage (on purpose)
@docusaurus/theme-common/internal private APIs on which we can safely do undocumented breaking changes

Test Plan

Preview keeps working

Test links

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

Related issues/PRs

v2.0 launch plan: #6113

netlify · 2022-06-22T17:13:32Z

✅ [V2]

Name	Link
🔨 Latest commit	`bb151c0`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/62b57eb90b44d00008b0154c
😎 Deploy Preview	https://deploy-preview-7660--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle 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 · 2022-06-22T17:14:11Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟢 93	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 86	🟢 100	🟢 100	🟢 100	🟢 90	Report

github-actions · 2022-06-22T17:18:49Z

Size Change: -4 B (0%)

Total Size: 802 kB

ℹ️ View Unchanged

Filename	Size	Change
`website/.docusaurus/globalData.json`	52.6 kB	0 B
`website/build/assets/css/styles.********.css`	107 kB	0 B
`website/build/assets/js/main.********.js`	604 kB	-4 B (0%)
`website/build/index.html`	38.9 kB	0 B

_{compressed-size-action}

Josh-Cena

My idea is we don't have to be too conservative about marking APIs as public. General utilities should definitely be public. I feel like we should have more public APIs than internal ones. I marked those that I have the strongest opinions about, but we need to think about every one of them, probably make a ranking.

Some other thoughts:

We definitely can select certain "interesting" utilities to document while only keeping others retro-compatible (i.e. safe for eject) without documentation
"Public" can mean three things:
- External theme authors are encouraged to use these
- Site creators are encouraged to use these in their custom pages
- Safe for swizzle eject

packages/docusaurus-theme-common/src/index.ts

packages/docusaurus-theme-common/src/internal.ts

Josh-Cena · 2022-06-22T17:19:44Z

packages/docusaurus-theme-common/src/internal.ts

+export {
+  useFilteredAndTreeifiedTOC,
+  useTreeifiedTOC,
+  type TOCTreeNode,
+} from './utils/tocUtils';


At least useTreeifiedTOC is very useful

useTreeifiedTOC is not used in any theme component, only useFilteredAndTreeifiedTOC is, which does feel like a weird name that we may want to change currently. Similarly, not sure i's useful to publicly commit to retro-compatibility on apis that even our own theme does not use. In the end we want the TOC components to be safe to swizzle, and in this case it doesn't help.

I would be ok to expose a unique useTOCTree(array, options) or something later, but this requires some refactoring.

I want to keep this PR clean from refactors: focusing only on splitting private/internal that we are ready to expose right now, as is.

What I mean is that we can do extra refactors in subsequent PRs, and it's not a big deal if this one API is not public immediately.

I once planned to document useTreeifiedTOC in #6729, because it's no longer possible to pick arbitrary TOC subtrees so users need to implement this themselves. But I agree the API is not perfect yet and can benefit from some more refactors.

packages/docusaurus-theme-common/src/internal.ts

packages/docusaurus-theme-common/src/index.ts

packages/docusaurus-theme-common/src/internal.ts

slorber · 2022-06-23T16:43:27Z

My idea is we don't have to be too conservative about marking APIs as public. General utilities should definitely be public. I feel like we should have more public APIs than internal ones.

Agree on that.
I just do not want to mark everything as public right now in this very PR so that we can at least move on and not delay merge with too many discussions.
But we can continue to move more internal APIs to public in subsequent PRs for sure 👍

Some other thoughts:

We definitely can select certain "interesting" utilities to document while only keeping others retro-compatible (i.e. safe for eject) without documentation

"Public" can mean three things:

External theme authors are encouraged to use these

Site creators are encouraged to use these in their custom pages

Safe for swizzle eject

Looks similar to what I had in mind, except I grouped the first 2 cases as "documented APIs" (~= encouraged usage)

Note this PR somehow split APIs to document from APIs to not document. We may not 100% agree on this split yet, but it's not a big deal as those are not documented yet 🤪 We'll refine that later on a case-by-case basis, for now I just wanted to have an initial set of public APIs extracted

slorber · 2022-06-23T20:01:52Z

packages/docusaurus-theme-common/removeThemeInternalReexport.mjs

+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import fs from 'fs-extra';
+
+// See comment in src/internal.ts
+// This script should be run by CI tests to remove:
+// export * from './index'
+
+const filePath = 'lib/internal.js';
+const lineToRemove = "export * from './index';\n";
+
+if (!(await fs.pathExists(filePath))) {
+  throw new Error(`internal entrypoint file not found at ${filePath}`);
+}
+
+const fileContent = await fs.readFile(filePath, 'utf8');
+
+const fileContentUpdated = fileContent.replaceAll(lineToRemove, '');
+
+// Ensure the script correctly removes the re-export
+if (fileContent === fileContentUpdated) {
+  throw new Error(
+    'Unexpected: internal re-export has not been replaced.\nMake sure this script works, and is only run once.',
+  );
+}
+
+await fs.writeFile(filePath, fileContentUpdated);


this idea is a bit convoluted but it looks to work fine 🤪

The error message is not very visible though 😓

Lol, this looks like pure hack... Since we are to automate the marking of ejection safety by analyzing dependency on /internal APIs, we can simply reuse the same analyzer. I'll investigate that later.

Yes 😅 maybe we'll remove this in the future once we have more public APIs

But this is not exactly the same problem: If a component is unsafe, it should still preferably import from the public endpoint instead of the internal re-export.

slorber · 2022-06-23T20:03:25Z

any concern to merge this?

We can move internal -> public and document things in subsequent PRs

Josh-Cena

Almost good—last concern.

Josh-Cena · 2022-06-24T04:10:56Z

packages/docusaurus-theme-common/src/index.ts

-  type NavbarSecondaryMenuComponent,
-} from './contexts/navbarSecondaryMenu/content';
-export {useNavbarSecondaryMenu} from './contexts/navbarSecondaryMenu/display';
+export {duplicates, uniq} from './utils/jsUtils';

 export {useBackToTopButton} from './hooks/useBackToTopButton';


Is this really stable? We have a lot of "wiring hooks" like useAnnouncementBar, useNavbarMobileSidebar, etc., and this is the only public one.

Moved to internal for now as I'm not 100% satisfied with the api, but we'll have to figure out how to make all these public on a case-by-case basis to ensure we can safely swizzle the related components.

Josh-Cena

LGTM—I think this is a good starting point and there's plenty to document already.

facebook/docusaurus#7660)

slorber added 2 commits June 22, 2022 17:15

add theme-common/internal export

bf7d73e

Split @docusaurus/theme-common into public/internal apis

e1109fa

slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jun 22, 2022

slorber requested review from lex111 and Josh-Cena as code owners June 22, 2022 17:08

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 22, 2022

slorber marked this pull request as draft June 22, 2022 17:09

fixes

5a6fa6c

Josh-Cena reviewed Jun 22, 2022

View reviewed changes

Josh-Cena changed the title ~~refactor(theme): split @docusaurus/theme-common into public/internal apis~~ refactor(theme): split @docusaurus/theme-common into public/internal APIs Jun 22, 2022

Josh-Cena changed the title ~~refactor(theme): split @docusaurus/theme-common into public/internal APIs~~ refactor(theme-common): split into public/internal APIs Jun 23, 2022

slorber added 5 commits June 23, 2022 17:00

Merge branch 'main' into slorber/theme-common-internal-export

24961bc

public <-> private

27b67ac

public <-> private

66ab86d

public <-> private

bcae7d8

fix

ca658b3

slorber added 3 commits June 23, 2022 19:19

add "removeThemeInternalReexport" CI script

73acc61

:s windows CI check not working: not that useful

73a2b1f

remove bad import

991a5aa

slorber marked this pull request as ready for review June 23, 2022 20:00

slorber commented Jun 23, 2022

View reviewed changes

Josh-Cena added 2 commits June 24, 2022 11:54

refactors

071fb3b

reorder

9bdb56d

Josh-Cena reviewed Jun 24, 2022

View reviewed changes

make useBackToTopButton internal

bb151c0

Josh-Cena approved these changes Jun 24, 2022

View reviewed changes

slorber changed the title ~~refactor(theme-common): split into public/internal APIs~~ refactor(theme-common): split package into public/internal API entrypoints Jun 24, 2022

slorber merged commit 9473508 into main Jun 24, 2022

slorber deleted the slorber/theme-common-internal-export branch June 24, 2022 09:21

slorber mentioned this pull request Jun 24, 2022

refactor(theme-common): rename useDynamicCallback to useEvent #7671

Merged

3 tasks

slorber mentioned this pull request Jul 8, 2022

chore: prepare v2.0.0-beta.22 release #7741

Merged

arisac added a commit to arisac/docusaurus-boilerplate that referenced this pull request Jul 11, 2022

docs: update extras import (useDocsSidebar hook moved to internal, ref:

3a04f07

facebook/docusaurus#7660)

ntucker mentioned this pull request Jul 17, 2022

docs(fix): Update imports to reflect breaking changes in beta.22 reactive/data-client#2088

Merged

jtoar mentioned this pull request Jul 25, 2022

fix(deps): update docusaurus monorepo to v2.0.0-rc.1 redwoodjs/redwood#5912

Merged

1 task

hsheth2 mentioned this pull request Aug 31, 2022

chore(docs-website): bump docusaurus to v2.1.0 datahub-project/datahub#5792

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(theme-common): split package into public/internal API entrypoints #7660

refactor(theme-common): split package into public/internal API entrypoints #7660

slorber commented Jun 22, 2022 •

edited

netlify bot commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

Josh-Cena left a comment •

edited

Josh-Cena Jun 22, 2022

slorber Jun 23, 2022

Josh-Cena Jun 24, 2022

slorber commented Jun 23, 2022 •

edited

slorber Jun 23, 2022 •

edited

Josh-Cena Jun 24, 2022

slorber Jun 24, 2022

slorber commented Jun 23, 2022

Josh-Cena left a comment

Josh-Cena Jun 24, 2022

slorber Jun 24, 2022

Josh-Cena left a comment

refactor(theme-common): split package into public/internal API entrypoints #7660

refactor(theme-common): split package into public/internal API entrypoints #7660

Conversation

slorber commented Jun 22, 2022 • edited

Breaking change

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

netlify bot commented Jun 22, 2022 • edited

✅ [V2]

github-actions bot commented Jun 22, 2022 • edited

⚡️ Lighthouse report for the deploy preview of this PR

github-actions bot commented Jun 22, 2022 • edited

Josh-Cena left a comment • edited

Choose a reason for hiding this comment

Josh-Cena Jun 22, 2022

Choose a reason for hiding this comment

slorber Jun 23, 2022

Choose a reason for hiding this comment

Josh-Cena Jun 24, 2022

Choose a reason for hiding this comment

slorber commented Jun 23, 2022 • edited

slorber Jun 23, 2022 • edited

Choose a reason for hiding this comment

Josh-Cena Jun 24, 2022

Choose a reason for hiding this comment

slorber Jun 24, 2022

Choose a reason for hiding this comment

slorber commented Jun 23, 2022

Josh-Cena left a comment

Choose a reason for hiding this comment

Josh-Cena Jun 24, 2022

Choose a reason for hiding this comment

slorber Jun 24, 2022

Choose a reason for hiding this comment

Josh-Cena left a comment

Choose a reason for hiding this comment

slorber commented Jun 22, 2022 •

edited

netlify bot commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

Josh-Cena left a comment •

edited

slorber commented Jun 23, 2022 •

edited

slorber Jun 23, 2022 •

edited