-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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-common shouldn't depend on docs content #10316
Merged
Merged
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
7f73e91
initial move
slorber d0005c7
Merge branch 'main' into slorber/move-docs-out-of-theme-common
slorber 34047ee
fix some imports
slorber 783b61b
refactor contextual search code
slorber 3a28456
stable refactor
slorber 1f09fed
Merge branch 'main' into slorber/move-docs-out-of-theme-common
slorber 758a2a4
use * for peerDep
slorber b91f4ca
typo
slorber e30e0ca
add missing removed public APIs
slorber 95eff77
try to fix yarn pnp tests
slorber 87221cc
try to fix yarn pnp tests
slorber 6360735
try to fix yarn pnp tests
slorber 1dfb2d9
try to fix yarn pnp tests
slorber 3ef0ee4
try to fix yarn pnp tests
slorber d870336
try to fix yarn pnp tests
slorber c6edbcd
try to fix yarn pnp tests
slorber b8f1c63
try to fix yarn pnp tests
slorber 1787ab9
try to fix yarn pnp tests
slorber e6ba17c
try to fix yarn pnp tests
slorber 65b840d
try to fix yarn pnp tests
slorber b719ef2
try to fix yarn pnp tests
slorber aebcb12
try to fix yarn pnp tests
slorber 89b6b5c
try to fix yarn pnp tests
slorber File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
packages/docusaurus-plugin-content-docs/src/client/docsSearch.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/** | ||
* 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 { | ||
useAllDocsData, | ||
useActivePluginAndVersion, | ||
} from '@docusaurus/plugin-content-docs/client'; | ||
import {useDocsPreferredVersionByPluginId} from './docsPreferredVersion'; | ||
|
||
/** The search tag to append as each doc's metadata. */ | ||
export function getDocsVersionSearchTag( | ||
pluginId: string, | ||
versionName: string, | ||
): string { | ||
return `docs-${pluginId}-${versionName}`; | ||
} | ||
|
||
/** | ||
* Gets the relevant docs tags to search. | ||
* This is the logic that powers the contextual search feature. | ||
* | ||
* If user is browsing Android 1.4 docs, he'll get presented with: | ||
* - Android '1.4' docs | ||
* - iOS 'preferred | latest' docs | ||
* | ||
* The result is generic and not coupled to Algolia/DocSearch on purpose. | ||
*/ | ||
export function useDocsContextualSearchTags(): string[] { | ||
const allDocsData = useAllDocsData(); | ||
const activePluginAndVersion = useActivePluginAndVersion(); | ||
const docsPreferredVersionByPluginId = useDocsPreferredVersionByPluginId(); | ||
|
||
// This can't use more specialized hooks because we are mapping over all | ||
// plugin instances. | ||
function getDocPluginTags(pluginId: string) { | ||
const activeVersion = | ||
activePluginAndVersion?.activePlugin.pluginId === pluginId | ||
? activePluginAndVersion.activeVersion | ||
: undefined; | ||
|
||
const preferredVersion = docsPreferredVersionByPluginId[pluginId]; | ||
|
||
const latestVersion = allDocsData[pluginId]!.versions.find( | ||
(v) => v.isLast, | ||
)!; | ||
|
||
const version = activeVersion ?? preferredVersion ?? latestVersion; | ||
|
||
return getDocsVersionSearchTag(pluginId, version.name); | ||
} | ||
|
||
return [...Object.keys(allDocsData).map(getDocPluginTags)]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @Josh-Cena remember how this yarn pnp thing works?
trying to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frankly, no—this looks correct but if it doesn't I'm not sure why.
However I'm not sure if such patching in CI is a good thing. We should either add that dependency ourselves or find a way to not do so (such as using a peer dependency and requiring pnp users to declare these as real dependencies).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't it what I do here, using a peerDependency already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @arcanis @clemyan would you mind helping us find the proper package extensions to get PnP working here?
Afaik you also use pnp on your own Yarn Docusaurus site, so you'd probably need to do the same on your site on next release
https://github.com/yarnpkg/berry/blame/master/.yarnrc.yml#L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package extensions only take effect during an install. For this specific case, moving the
yarn add
below theyarn config set
s will probably resolve it.As far as I can tell, the current release of
preset-classic
works out-of-the-box for PnP (We need package extensions on our end only for integration withplugin-typedoc-api
), and this change breaks that. So there can be a bigger discussion around supporting PnP if you want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Will try to run this locally and see if I can fix it because CI debugging is not super effective 😅
Regarding supporting PnP out of the box I'd be happy to if we can without extensions.
In this PR, the problem is that:
@docusaurus/theme-common
to@docusaurus/plugin-content-docs/client
@docusaurus/theme-common
depended on@docusaurus/plugin-content-docs
@docusaurus/plugin-content-docs
depend on@docusaurus/theme-common
@docusaurus/theme-common
, only temporarily for retro-compatibility reasons (will be removed in v4)Any idea how to do so and still play well with PnP?
Note that this is temporary so if we remove this thing in V4 Docusaurus should work again better in PnP I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have already made that change for
plugin-content-blog
and presumably you will do so forplugin-content-pages
.I would say the "correct" way to do this during the transition would be
theme-common
declares optional peer deps on all 3 content pluginstheme-common
and optional peer deps on the other two content pluginsThat way, dependents of
preset-classic
ortheme-classic
would see no difference. (Unless the user usesoverrides
orresolutions
, but then the onus is on them)However, there is the weirdness of having content plugins peer-depend on each other. (though the end users likely wouldn't notice because they are optional)
Also, if someone depends directly on
theme-common
, they could, for example, use the docs-specific APIs without depending onplugin-content-docs
themself. Now they would get a runtime error. Given that this would be a very obscure usecase, I will leave it up to you to decide whether this counts as a breaking change.Another way to do this is to embrace circularly-dependent packages. During the transition, have
theme-common
and the 3 content plugins depend on each other. Given your dependencies are declared with exact versions, there is minimal risk of conflicts. Yarn can deal with circular packages, and I strongly believe npm and pnpm also can. (I think babel also has a few circular packages)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @clemyan !
I tried adding docs as a blog peerDeps and apparently it's enough to fix the problem.
Do you think this is a good enough temporary change? Or do you think I should apply your full suggestion (3 plugins depending on each other). I'd prefer to have a minimal solution if possible.
Regarding cyclic dependencies, afaik our TS monorepo (using Lerna) needs to build packages in the correct order, defined by dependencies. I don't think tsc can build one package if its dependency types hasn't been built first, so it would probably writing
.d.ts
files manually to solve it, or use the new Isolated Declarations thing from TS 5.5. I'd prefer not having a cycle for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further inspection,
plugins-content-pages
has no interaction withtheme-common
at all? Andtheme-common
only needsplugin-content-docs
for BC?If that's the case, then yes, the peer dependencies you declared should be sufficient. Though ideally they should be marked optional via
peerDependenciesMeta
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for now our page plugin doesn't require theme-common and theme-common only needs docs temporarily for retrocompatibility.
Thanks for your help solving this!