feat(content-docs): versionedDocsPath config for docs plugin #8076

maxhr · 2022-09-09T14:57:46Z

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

Addresses #8061 where you can configure a directory to write the versions to when running docusaurus docs:version.

A new parameter in content-docs plugin config versionedDocsPath, by default it's undefined and we use the current behavior and write versions to site dir.

It supports one versionedDocsPath per plugin instance, but not dirs per-version as described in the issue.

Test Plan

Added unit tests for new configuration.

Test links

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

Related issues/PRs

netlify · 2022-09-09T15:03:10Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`cb35a1a`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/631b546ce196ab000881c7cd
😎 Deploy Preview	https://deploy-preview-8076--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.

Josh-Cena · 2022-09-09T15:03:42Z

Wow, that's a big one (I know, probably a lot of test files). I'm quite busy these days and will be slow at getting to it, while @slorber is on holiday till the end of this month (and he needs to be the one to approve new public API surface). Sorry if we are slow to review–we will eventually give feedback!

github-actions · 2022-09-09T15:03:52Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 74	🟢 98	🟢 100	🟢 100	🟠 80	Report
/docs/installation	🟠 79	🟢 100	🟢 100	🟢 100	🟢 90	Report

slorber

Thanks, looks like a great start 👍

But there are many details I'd like to see changed 🤪

For some reason GitHub doesn't render diff properly on the CLI tests so I can't review through GitHub, will have to check the code locally 😓

slorber · 2022-09-30T16:02:52Z

packages/docusaurus-plugin-content-docs/src/versions/files.ts

  versionName: string,
 ): string {
  return path.join(
    siteDir,
+    versionedDocsPath || '',


I'd rather avoid such default values scattered in multiple places: move them to option normalization process or even better option defaults

If versionedDocsPath should remain undefined in option, I'd rather have a getVersionedDocsPath(siteDir, versionedDocsPathOption) to apply the fallback in a single place

slorber · 2022-09-30T16:07:46Z

website/docs/api/plugins/plugin-content-docs.md

@@ -34,6 +34,7 @@ Accepted fields:
 | Name | Type | Default | Description |
 | --- | --- | --- | --- |
 | `path` | `string` | `'docs'` | Path to the docs content directory on the file system, relative to site directory. |
+| `versionedDocsPath` | `string` | `undefined` | Path to the versioned docs content directory on the file system, relative to site directory. This is where the CLI command `docusaurus docs:version 1.2.3` will write the files to. The default is the site directory. |


It was not clear to me at all reading this doc that the default value is not website/versioned_docs but rather simply website and that it affects where both versioned_docs and versioned_sidebars will be written.

Wondering if it wouldn't be better to have 2 standalone options to give more flexibility? 🤷‍♂️

slorber · 2022-09-30T16:11:07Z

packages/docusaurus-plugin-content-docs/src/versions/files.ts

  versionName: string,
 ): string {
  return path.join(
    siteDir,
+    versionedDocsPath || '',


what if the user provides an absolute versionedDocsPath ?

slorber · 2022-09-30T16:20:20Z

packages/docusaurus-plugin-content-docs/src/versions/__tests__/index.test.ts

@@ -758,4 +758,136 @@ describe('readVersionsMetadata', () => {
      );
    });
  });
+
+  describe('versioned site with custom versions dir, pluginId=community', () => {


It feels overkill to add all these new tests just for testing that particular case: lots of boilerplate for little being tested

Why not simply creating a new entry

it('works with relative versionedDocsPath', async () => { const {defaultOptions, defaultContext, vCurrent} = await loadSite(); const versionsMetadata = await readVersionsMetadata({ options: {...defaultOptions, versionedDocsPath: './community-versions'}, context: defaultContext, }); expect(versionsMetadata).toEqual([ { ...vCurrent, contentPath: "...", sidebarFilePath: "...", }, ]); )}

This should test it without so much ceremony

Also please test with absolute paths?

slorber · 2022-09-30T16:21:17Z

packages/docusaurus-plugin-content-docs/src/plugin-content-docs.d.ts

+    /**
+     * Path to the versioned docs content directory on the file system, relative to site
+     * directory. Running the tagging script `docusaurus docs:version X` will generate
+     * the files in this directory. The default is the site directory.
+     */
+    versionedDocsPath?: string | undefined;


again not 100% agreeing with the comment: Docusaurus won't generate files in this directory but rather in 2 subdirectories of this directory (as far as I understand it)

And if absolute paths are not supported, option validation should reject it. But absolute paths should probably be supported

slorber · 2022-09-30T16:25:47Z

packages/docusaurus-plugin-content-docs/src/versions/files.ts

    addPluginIdPrefix(VERSIONED_DOCS_DIR, pluginId),
    `version-${versionName}`,
  );
 }

-/** `[siteDir]/community_versioned_sidebars/version-1.0.0-sidebars.json` */
+/** `[siteDir]/[versionedDocsPath]/community_versioned_sidebars/version-1.0.0-sidebars.json` */


This comment assumes the path will be relative. Maybe it will be absolute in which case we don't need the [siteDir] prefix.

Maybe the siteDir does not need anymore to be passed to this function? path.join(versionedDocsPath,...rest) could be enough?

slorber · 2022-09-30T16:32:57Z

.../docusaurus-plugin-content-docs/src/__tests__/__fixtures__/empty-site-2/docusaurus.config.js

+ * LICENSE file in the root directory of this source tree.
+ */
+
+module.exports = {


Not sure what's the purpose of this empty-site-2: in which test is this used?

slorber · 2022-09-30T16:40:58Z

packages/docusaurus-plugin-content-docs/src/__tests__/index.test.ts

@@ -584,6 +584,119 @@ describe('versioned website (community)', () => {
  });
 });

+describe('versioned website (community) with custom versions dir', () => {


What about having a parametrized loadSite({versionedDocsPath: "community-versions"})?

Wonder if it wouldn't simply our test a bit as you could reuse the loadSite() that already existed in the other describe?

I guess the initial idea was to have 1 describe per site fixture

slorber · 2022-09-30T16:43:12Z

packages/docusaurus-plugin-content-docs/src/__tests__/index.test.ts

+      options: {
+        id: 'community',
+        path: 'community',
+        versionedDocsPath: 'community-versions',


maybe make the path more explicit? like community_custom_versioned_docs_path?

Having a site with multiple versionedDocsPath (default + this one) is already a bit hard to reason about, there's a chance in 1 year I get really confused by all this 🤪

slorber · 2022-09-30T16:47:49Z

...ugin-content-docs/src/__tests__/__fixtures__/versioned-sites-2/v-site-1/docusaurus.config.js

@@ -0,0 +1,16 @@
+/**


I don't really like the name versioned-sites-2: when browsing that doc, I have no idea why there is a "2" in the name and what's the purpose of this fixture.

Please make it explicit, maybe rename versioned-sites-2 to versioned_docs_path_fixtures or something? don't hesitate to nest the site + its associated content in a subfolder so that we understand this association better

Jersyfi · 2023-02-27T15:06:31Z

@maxhr @slorber is any help needed here to get progress on this PR? I need this feature to proceed with my documentation because I have several frameworks with different versioning which looks messy if they aren't in a single folder.

slorber · 2023-03-02T14:13:22Z

@maxhr @slorber is any help needed here to get progress on this PR? I need this feature to proceed with my documentation because I have several frameworks with different versioning which looks messy if they aren't in a single folder.

I left a review that wasn't handled yet and should focus on other PRs (notable MDX2 upgrade).

If @maxhr doesn't want to finish this PR, you can probably fork his branch (so that @maxhr remains credited for his work), continue the work, handle my review, and open a new PR that I can review.

Jersyfi · 2023-03-02T22:27:28Z

@slorber i will take a close look at this PR if it makes sense for me to fork it. Otherwise i will request my own PR.

feat(content-docs): versionedDocsPath config for docs plugin

cb35a1a

maxhr requested review from slorber, lex111 and Josh-Cena as code owners September 9, 2022 14:57

facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 9, 2022

Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Sep 9, 2022

slorber requested changes Sep 30, 2022

View reviewed changes

slorber marked this pull request as draft September 30, 2022 16:50

Jersyfi mentioned this pull request Feb 27, 2023

Multi Framework Docs Plugin Jersyfi/cookify-docs#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(content-docs): versionedDocsPath config for docs plugin #8076

feat(content-docs): versionedDocsPath config for docs plugin #8076

maxhr commented Sep 9, 2022 •

edited

Loading

netlify bot commented Sep 9, 2022

Josh-Cena commented Sep 9, 2022 •

edited

Loading

github-actions bot commented Sep 9, 2022

slorber left a comment

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

slorber Sep 30, 2022

Jersyfi commented Feb 27, 2023

slorber commented Mar 2, 2023

Jersyfi commented Mar 2, 2023

feat(content-docs): versionedDocsPath config for docs plugin #8076

Are you sure you want to change the base?

feat(content-docs): versionedDocsPath config for docs plugin #8076

Conversation

maxhr commented Sep 9, 2022 • edited Loading

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

netlify bot commented Sep 9, 2022

✅ [V2]

Josh-Cena commented Sep 9, 2022 • edited Loading

github-actions bot commented Sep 9, 2022

⚡️ Lighthouse report for the deploy preview of this PR

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jersyfi commented Feb 27, 2023

slorber commented Mar 2, 2023

Jersyfi commented Mar 2, 2023

maxhr commented Sep 9, 2022 •

edited

Loading

Josh-Cena commented Sep 9, 2022 •

edited

Loading