refactor(docs): theme-common shouldn't depend on docs content #10316
Conversation
slorber
added
the
pr: maintenance
⚡️ Lighthouse report for the deploy preview of this PR
|
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: +688 B (+0.01%) Total Size: 10.9 MB
ℹ️ View Unchanged
|
.github/workflows/tests-e2e.yml
Outdated
| yarn config set packageExtensions --json '{ "@docusaurus/theme-common@*": { "dependencies": { "@docusaurus/plugin-content-docs": "*" } } }' | ||
| yarn config set packageExtensions --json '{ "@docusaurus/plugin-content-blog@*": { "dependencies": { "@docusaurus/plugin-content-docs": "*" } } }' |
There was a problem hiding this comment.
hey @Josh-Cena remember how this yarn pnp thing works?
trying to fix
[success] [webpackbar] Client: Compiled with some errors in 8.99s
Module not found: Error: @docusaurus/theme-common tried to access @docusaurus/plugin-content-docs (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.
Required package: @docusaurus/plugin-content-docs
Required by: @docusaurus/theme-common@virtual:6e94eb2be8bb440c1581853dd5cec23b82b14cece8f72bb270f25591c11b4fddad6899fee76fcf8f5b8405f8ff5008c1f7d44fc53a24a1945af23a31a750e3dd#npm:3.4.0-NEW (via /home/runner/work/docusaurus/test-website/.yarn/__virtual__/@docusaurus-theme-common-virtual-725f925b63/4/.yarn/berry/cache/@docusaurus-theme-common-npm-3.4.0-NEW-ae14abdd68-10c0.zip/node_modules/@docusaurus/theme-common/lib/)
Ancestor breaking the chain: @docusaurus/plugin-content-blog@virtual:311126612085e579a3976bffcf0c8410a5b0a22c5[17](https://github.com/facebook/docusaurus/actions/runs/10013274539/job/27680514170?pr=10316#step:9:18)e11607d2bb9fd078431ffba07e4aba5308c7bd73ed15cb0354b2679771dbf6971367c0afad10951965957#npm:3.4.0-NEW
Error: E2E_TEST: Project has compiler errors.
Error: Process completed with exit code 1.
There was a problem hiding this comment.
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.
But isn't it what I do here, using a peerDependency already?
There was a problem hiding this comment.
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.
Package extensions only take effect during an install. For this specific case, moving the yarn add below the yarn config sets 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 with plugin-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.
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:
- I want to move some docs-related public APIs from
@docusaurus/theme-commonto@docusaurus/plugin-content-docs/client - Previously
@docusaurus/theme-commondepended on@docusaurus/plugin-content-docs - Now
@docusaurus/plugin-content-docsdepend on@docusaurus/theme-common - I'd like to use a peer dependency to re-export the moved public APIs from
@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.
I see you have already made that change for plugin-content-blog and presumably you will do so for plugin-content-pages.
I would say the "correct" way to do this during the transition would be
theme-commondeclares optional peer deps on all 3 content plugins- The 3 content plugins declare dependency on
theme-commonand optional peer deps on the other two content plugins
That way, dependents of preset-classic or theme-classic would see no difference. (Unless the user uses overrides or resolutions, 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 on plugin-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.
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.
Upon further inspection, plugins-content-pages has no interaction with theme-common at all? And theme-common only needs plugin-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.
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!
|
Does this fix #7207 or is that issue really worth fixing? |
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:
Motivation
Follow-up of #10313
Note that I added a retro compatibility workaround for APIs from
@docusaurus/theme-commonthat have been moved around. They are public API surfaces although onlyuseCurrentSidebarCategoryhas ever been documented before v2.1 (not anymore)TODO as a v4 breaking change:
Test Plan
CI + preview
Test links
https://deploy-preview-10316--docusaurus-2.netlify.app/
Related issues/PRs