From 97958402f80120e60a791e4b73f54d6f3c03595c Mon Sep 17 00:00:00 2001 From: Kelly Joseph Price Date: Wed, 14 Feb 2024 16:56:08 -0800 Subject: [PATCH] feat: sort when syncing by parent (#973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit | 🚥 Resolves RM-8336 | | :------------------ | ## 🧰 Changes When syncing docs, it sorts them by their parents! Users reported issues where if a parent document was in a directory, it could error out because it would get synced after one of its children. ## 🧬 QA & Testing Not sure :grimacing:. --------- Co-authored-by: Kanad Gupta <8854718+kanadgupta@users.noreply.github.com> --- .../docs/docs-with-parent-ids/child.md | 6 + .../docs/docs-with-parent-ids/friend.md | 5 + .../docs/docs-with-parent-ids/parent.md | 5 + .../docs-with-parent-ids/with-parent-doc.md | 6 + .../docs/multiple-docs-cycle/child.md | 6 + .../docs/multiple-docs-cycle/grandparent.md | 6 + .../docs/multiple-docs-cycle/parent.md | 6 + .../docs/multiple-docs-no-parents/child.md | 6 + .../docs/multiple-docs-no-parents/friend.md | 5 + .../__fixtures__/docs/multiple-docs/child.md | 6 + .../__fixtures__/docs/multiple-docs/friend.md | 5 + .../docs/multiple-docs/grandparent.md | 5 + .../__fixtures__/docs/multiple-docs/parent.md | 6 + __tests__/cmds/docs/multiple.test.ts | 170 ++++++++++++++++++ package-lock.json | 13 ++ package.json | 2 + src/lib/readDoc.ts | 18 +- src/lib/syncDocsPath.ts | 44 ++++- 18 files changed, 307 insertions(+), 13 deletions(-) create mode 100644 __tests__/__fixtures__/docs/docs-with-parent-ids/child.md create mode 100644 __tests__/__fixtures__/docs/docs-with-parent-ids/friend.md create mode 100644 __tests__/__fixtures__/docs/docs-with-parent-ids/parent.md create mode 100644 __tests__/__fixtures__/docs/docs-with-parent-ids/with-parent-doc.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs-cycle/child.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs-cycle/grandparent.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs-cycle/parent.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs-no-parents/child.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs-no-parents/friend.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs/child.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs/friend.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs/grandparent.md create mode 100644 __tests__/__fixtures__/docs/multiple-docs/parent.md create mode 100644 __tests__/cmds/docs/multiple.test.ts diff --git a/__tests__/__fixtures__/docs/docs-with-parent-ids/child.md b/__tests__/__fixtures__/docs/docs-with-parent-ids/child.md new file mode 100644 index 000000000..216aead7e --- /dev/null +++ b/__tests__/__fixtures__/docs/docs-with-parent-ids/child.md @@ -0,0 +1,6 @@ +--- +title: Child +parentDocSlug: parent +--- + +# Child Body diff --git a/__tests__/__fixtures__/docs/docs-with-parent-ids/friend.md b/__tests__/__fixtures__/docs/docs-with-parent-ids/friend.md new file mode 100644 index 000000000..f406e500e --- /dev/null +++ b/__tests__/__fixtures__/docs/docs-with-parent-ids/friend.md @@ -0,0 +1,5 @@ +--- +title: Friend +--- + +# Friend Body diff --git a/__tests__/__fixtures__/docs/docs-with-parent-ids/parent.md b/__tests__/__fixtures__/docs/docs-with-parent-ids/parent.md new file mode 100644 index 000000000..385222605 --- /dev/null +++ b/__tests__/__fixtures__/docs/docs-with-parent-ids/parent.md @@ -0,0 +1,5 @@ +--- +title: Parent +--- + +# Parent Body diff --git a/__tests__/__fixtures__/docs/docs-with-parent-ids/with-parent-doc.md b/__tests__/__fixtures__/docs/docs-with-parent-ids/with-parent-doc.md new file mode 100644 index 000000000..c86849dcd --- /dev/null +++ b/__tests__/__fixtures__/docs/docs-with-parent-ids/with-parent-doc.md @@ -0,0 +1,6 @@ +--- +title: With Parent Doc +parentDoc: 1234 +--- + +# With Parent Doc Body diff --git a/__tests__/__fixtures__/docs/multiple-docs-cycle/child.md b/__tests__/__fixtures__/docs/multiple-docs-cycle/child.md new file mode 100644 index 000000000..216aead7e --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs-cycle/child.md @@ -0,0 +1,6 @@ +--- +title: Child +parentDocSlug: parent +--- + +# Child Body diff --git a/__tests__/__fixtures__/docs/multiple-docs-cycle/grandparent.md b/__tests__/__fixtures__/docs/multiple-docs-cycle/grandparent.md new file mode 100644 index 000000000..382b680c7 --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs-cycle/grandparent.md @@ -0,0 +1,6 @@ +--- +title: Grandparent +parentDocSlug: child +--- + +# Grandparent Body diff --git a/__tests__/__fixtures__/docs/multiple-docs-cycle/parent.md b/__tests__/__fixtures__/docs/multiple-docs-cycle/parent.md new file mode 100644 index 000000000..0efe16cfb --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs-cycle/parent.md @@ -0,0 +1,6 @@ +--- +title: Parent +parentDocSlug: grandparent +--- + +# Parent Body diff --git a/__tests__/__fixtures__/docs/multiple-docs-no-parents/child.md b/__tests__/__fixtures__/docs/multiple-docs-no-parents/child.md new file mode 100644 index 000000000..216aead7e --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs-no-parents/child.md @@ -0,0 +1,6 @@ +--- +title: Child +parentDocSlug: parent +--- + +# Child Body diff --git a/__tests__/__fixtures__/docs/multiple-docs-no-parents/friend.md b/__tests__/__fixtures__/docs/multiple-docs-no-parents/friend.md new file mode 100644 index 000000000..f406e500e --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs-no-parents/friend.md @@ -0,0 +1,5 @@ +--- +title: Friend +--- + +# Friend Body diff --git a/__tests__/__fixtures__/docs/multiple-docs/child.md b/__tests__/__fixtures__/docs/multiple-docs/child.md new file mode 100644 index 000000000..216aead7e --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs/child.md @@ -0,0 +1,6 @@ +--- +title: Child +parentDocSlug: parent +--- + +# Child Body diff --git a/__tests__/__fixtures__/docs/multiple-docs/friend.md b/__tests__/__fixtures__/docs/multiple-docs/friend.md new file mode 100644 index 000000000..f406e500e --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs/friend.md @@ -0,0 +1,5 @@ +--- +title: Friend +--- + +# Friend Body diff --git a/__tests__/__fixtures__/docs/multiple-docs/grandparent.md b/__tests__/__fixtures__/docs/multiple-docs/grandparent.md new file mode 100644 index 000000000..5c7664f23 --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs/grandparent.md @@ -0,0 +1,5 @@ +--- +title: Grandparent +--- + +# Grandparent Body diff --git a/__tests__/__fixtures__/docs/multiple-docs/parent.md b/__tests__/__fixtures__/docs/multiple-docs/parent.md new file mode 100644 index 000000000..0efe16cfb --- /dev/null +++ b/__tests__/__fixtures__/docs/multiple-docs/parent.md @@ -0,0 +1,6 @@ +--- +title: Parent +parentDocSlug: grandparent +--- + +# Parent Body diff --git a/__tests__/cmds/docs/multiple.test.ts b/__tests__/cmds/docs/multiple.test.ts new file mode 100644 index 000000000..8ff042710 --- /dev/null +++ b/__tests__/cmds/docs/multiple.test.ts @@ -0,0 +1,170 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +import frontMatter from 'gray-matter'; +import nock from 'nock'; +import { describe, beforeAll, afterAll, afterEach, it, expect, vi } from 'vitest'; + +import DocsCommand from '../../../src/cmds/docs/index.js'; +import getAPIMock, { getAPIMockWithVersionHeader } from '../../helpers/get-api-mock.js'; +import hashFileContents from '../../helpers/hash-file-contents.js'; + +const docs = new DocsCommand(); + +const fixturesBaseDir = '__fixtures__/docs'; +const fullFixturesDir = `${__dirname}./../../${fixturesBaseDir}`; + +const key = 'API_KEY'; +const version = '1.0.0'; + +describe('rdme docs (multiple)', () => { + beforeAll(() => { + nock.disableNetConnect(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + afterAll(() => nock.cleanAll()); + + it('should upload parent docs first', async () => { + const dir = 'multiple-docs'; + const slugs = ['grandparent', 'parent', 'child', 'friend']; + let id = 1234; + + const mocks = slugs.flatMap(slug => { + const doc = frontMatter(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + const hash = hashFileContents(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + + return [ + getAPIMockWithVersionHeader(version) + .get(`/api/v1/docs/${slug}`) + .basicAuth({ user: key }) + .reply(404, { + error: 'DOC_NOTFOUND', + message: `The doc with the slug '${slug}' couldn't be found`, + suggestion: '...a suggestion to resolve the issue...', + help: 'If you need help, email support@readme.io and mention log "fake-metrics-uuid".', + }), + getAPIMockWithVersionHeader(version) + .post('/api/v1/docs', { slug, body: doc.content, ...doc.data, lastUpdatedHash: hash }) + .basicAuth({ user: key }) + // eslint-disable-next-line no-plusplus + .reply(201, { slug, _id: id++, body: doc.content, ...doc.data, lastUpdatedHash: hash }), + ]; + }); + + const versionMock = getAPIMock().get(`/api/v1/version/${version}`).basicAuth({ user: key }).reply(200, { version }); + + const promise = docs.run({ filePath: `./__tests__/${fixturesBaseDir}/${dir}`, key, version }); + + await expect(promise).resolves.toStrictEqual( + [ + `🌱 successfully created 'friend' (ID: 1237) with contents from __tests__/${fixturesBaseDir}/${dir}/friend.md`, + `🌱 successfully created 'grandparent' (ID: 1234) with contents from __tests__/${fixturesBaseDir}/${dir}/grandparent.md`, + `🌱 successfully created 'parent' (ID: 1235) with contents from __tests__/${fixturesBaseDir}/${dir}/parent.md`, + `🌱 successfully created 'child' (ID: 1236) with contents from __tests__/${fixturesBaseDir}/${dir}/child.md`, + ].join('\n'), + ); + + mocks.forEach(mock => mock.done()); + versionMock.done(); + }); + + it('should upload docs with parent doc ids first', async () => { + const dir = 'docs-with-parent-ids'; + const slugs = ['child', 'friend', 'with-parent-doc', 'parent']; + let id = 1234; + + const mocks = slugs.flatMap(slug => { + const doc = frontMatter(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + const hash = hashFileContents(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + + return [ + getAPIMockWithVersionHeader(version) + .get(`/api/v1/docs/${slug}`) + .basicAuth({ user: key }) + .reply(404, { + error: 'DOC_NOTFOUND', + message: `The doc with the slug '${slug}' couldn't be found`, + suggestion: '...a suggestion to resolve the issue...', + help: 'If you need help, email support@readme.io and mention log "fake-metrics-uuid".', + }), + getAPIMockWithVersionHeader(version) + .post('/api/v1/docs', { slug, body: doc.content, ...doc.data, lastUpdatedHash: hash }) + .basicAuth({ user: key }) + // eslint-disable-next-line no-plusplus + .reply(201, { slug, _id: id++, body: doc.content, ...doc.data, lastUpdatedHash: hash }), + ]; + }); + + const versionMock = getAPIMock().get(`/api/v1/version/${version}`).basicAuth({ user: key }).reply(200, { version }); + + const promise = docs.run({ filePath: `./__tests__/${fixturesBaseDir}/${dir}`, key, version }); + + await expect(promise).resolves.toStrictEqual( + [ + `🌱 successfully created 'with-parent-doc' (ID: 1236) with contents from __tests__/${fixturesBaseDir}/${dir}/with-parent-doc.md`, + `🌱 successfully created 'friend' (ID: 1235) with contents from __tests__/${fixturesBaseDir}/${dir}/friend.md`, + `🌱 successfully created 'parent' (ID: 1237) with contents from __tests__/${fixturesBaseDir}/${dir}/parent.md`, + `🌱 successfully created 'child' (ID: 1234) with contents from __tests__/${fixturesBaseDir}/${dir}/child.md`, + ].join('\n'), + ); + + mocks.forEach(mock => mock.done()); + versionMock.done(); + }); + + it('should upload child docs without the parent', async () => { + const dir = 'multiple-docs-no-parents'; + const slugs = ['child', 'friend']; + let id = 1234; + + const mocks = slugs.flatMap(slug => { + const doc = frontMatter(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + const hash = hashFileContents(fs.readFileSync(path.join(fullFixturesDir, `/${dir}/${slug}.md`))); + + return [ + getAPIMockWithVersionHeader(version) + .get(`/api/v1/docs/${slug}`) + .basicAuth({ user: key }) + .reply(404, { + error: 'DOC_NOTFOUND', + message: `The doc with the slug '${slug}' couldn't be found`, + suggestion: '...a suggestion to resolve the issue...', + help: 'If you need help, email support@readme.io and mention log "fake-metrics-uuid".', + }), + getAPIMockWithVersionHeader(version) + .post('/api/v1/docs', { slug, body: doc.content, ...doc.data, lastUpdatedHash: hash }) + .basicAuth({ user: key }) + // eslint-disable-next-line no-plusplus + .reply(201, { slug, _id: id++, body: doc.content, ...doc.data, lastUpdatedHash: hash }), + ]; + }); + + const versionMock = getAPIMock().get(`/api/v1/version/${version}`).basicAuth({ user: key }).reply(200, { version }); + + const promise = docs.run({ filePath: `./__tests__/${fixturesBaseDir}/${dir}`, key, version }); + + await expect(promise).resolves.toStrictEqual( + [ + `🌱 successfully created 'child' (ID: 1234) with contents from __tests__/${fixturesBaseDir}/${dir}/child.md`, + `🌱 successfully created 'friend' (ID: 1235) with contents from __tests__/${fixturesBaseDir}/${dir}/friend.md`, + ].join('\n'), + ); + + mocks.forEach(mock => mock.done()); + versionMock.done(); + }); + + it('should return an error message when it encounters a cycle', async () => { + const dir = 'multiple-docs-cycle'; + const versionMock = getAPIMock().get(`/api/v1/version/${version}`).basicAuth({ user: key }).reply(200, { version }); + + const promise = docs.run({ filePath: `./__tests__/${fixturesBaseDir}/${dir}`, key, version }); + + await expect(promise).rejects.toThrow('Cyclic dependency'); + versionMock.done(); + }); +}); diff --git a/package-lock.json b/package-lock.json index f86984b02..0847ce26b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -33,6 +33,7 @@ "string-argv": "^0.3.1", "table": "^6.8.1", "tmp-promise": "^3.0.2", + "toposort": "^2.0.2", "update-notifier": "^7.0.0", "validator": "^13.7.0" }, @@ -59,6 +60,7 @@ "@types/pluralize": "^0.0.33", "@types/prompts": "^2.4.2", "@types/semver": "^7.3.12", + "@types/toposort": "^2.0.7", "@types/update-notifier": "^6.0.5", "@types/validator": "^13.7.6", "@vitest/coverage-v8": "^1.1.0", @@ -2853,6 +2855,12 @@ "integrity": "sha512-Hy6UMpxhE3j1tLpl27exp1XqHD7n8chAiNPzWfz16LPZoMMoSc4dzLl6w9qijkEb/r5O1ozdu1CWGA2L83ZeZg==", "dev": true }, + "node_modules/@types/toposort": { + "version": "2.0.7", + "resolved": "https://registry.npmjs.org/@types/toposort/-/toposort-2.0.7.tgz", + "integrity": "sha512-sQNk65vbC36+UixCkcky+dCr7MlflHcVILg1FVGqlUntsLFv9xd9ToWIVko/gTuin+cVe16t+2YubEFkhnSuPQ==", + "dev": true + }, "node_modules/@types/unist": { "version": "2.0.10", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-2.0.10.tgz", @@ -15829,6 +15837,11 @@ "url": "https://opencollective.com/unified" } }, + "node_modules/toposort": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/toposort/-/toposort-2.0.2.tgz", + "integrity": "sha512-0a5EOkAUp8D4moMi2W8ZF8jcga7BgZd91O/yabJCFY8az+XSzeGyTKs0Aoo897iV1Nj6guFq8orWDS96z91oGg==" + }, "node_modules/tr46": { "version": "0.0.3", "resolved": "https://registry.npmjs.org/tr46/-/tr46-0.0.3.tgz", diff --git a/package.json b/package.json index aa8a0350f..d5768707d 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "string-argv": "^0.3.1", "table": "^6.8.1", "tmp-promise": "^3.0.2", + "toposort": "^2.0.2", "update-notifier": "^7.0.0", "validator": "^13.7.0" }, @@ -83,6 +84,7 @@ "@types/pluralize": "^0.0.33", "@types/prompts": "^2.4.2", "@types/semver": "^7.3.12", + "@types/toposort": "^2.0.7", "@types/update-notifier": "^6.0.5", "@types/validator": "^13.7.6", "@vitest/coverage-v8": "^1.1.0", diff --git a/src/lib/readDoc.ts b/src/lib/readDoc.ts index 1e23644a9..e74b8a0ef 100644 --- a/src/lib/readDoc.ts +++ b/src/lib/readDoc.ts @@ -6,11 +6,13 @@ import grayMatter from 'gray-matter'; import { debug } from './logger.js'; -interface ReadDocMetadata { +export interface ReadDocMetadata { /** The contents of the file below the YAML front matter */ content: string; /** A JSON object with the YAML front matter */ data: Record; + /** The original filePath */ + filePath: string; /** * A hash of the file contents (including the front matter) */ @@ -22,19 +24,19 @@ interface ReadDocMetadata { /** * Returns the content, matter and slug of the specified Markdown or HTML file * - * @param {String} filepath path to the HTML/Markdown file + * @param {String} filePath path to the HTML/Markdown file * (file extension must end in `.html`, `.md`., or `.markdown`) */ -export default function readDoc(filepath: string): ReadDocMetadata { - debug(`reading file ${filepath}`); - const rawFileContents = fs.readFileSync(filepath, 'utf8'); +export default function readDoc(filePath: string): ReadDocMetadata { + debug(`reading file ${filePath}`); + const rawFileContents = fs.readFileSync(filePath, 'utf8'); const matter = grayMatter(rawFileContents); const { content, data } = matter; - debug(`front matter for ${filepath}: ${JSON.stringify(matter)}`); + debug(`front matter for ${filePath}: ${JSON.stringify(matter)}`); // Stripping the subdirectories and markdown extension from the filename and lowercasing to get the default slug. - const slug = matter.data.slug || path.basename(filepath).replace(path.extname(filepath), '').toLowerCase(); + const slug = matter.data.slug || path.basename(filePath).replace(path.extname(filePath), '').toLowerCase(); const hash = crypto.createHash('sha1').update(rawFileContents).digest('hex'); - return { content, data, hash, slug }; + return { content, data, filePath, hash, slug }; } diff --git a/src/lib/syncDocsPath.ts b/src/lib/syncDocsPath.ts index 2e816b01e..8701cf6b1 100644 --- a/src/lib/syncDocsPath.ts +++ b/src/lib/syncDocsPath.ts @@ -1,8 +1,11 @@ +import type { ReadDocMetadata } from './readDoc.js'; + import fs from 'node:fs/promises'; import path from 'node:path'; import chalk from 'chalk'; import { Headers } from 'node-fetch'; +import toposort from 'toposort'; import APIError from './apiError.js'; import Command, { CommandCategories } from './baseCommand.js'; @@ -27,10 +30,10 @@ async function pushDoc( key: string, selectedVersion: string | undefined, dryRun: boolean, - filePath: string, + fileData: ReadDocMetadata, type: CommandCategories, ) { - const { content, data, hash, slug } = readDoc(filePath); + const { content, data, filePath, hash, slug } = fileData; // TODO: ideally we should offer a zero-configuration approach that doesn't // require YAML front matter, but that will have to be a breaking change @@ -127,6 +130,28 @@ async function pushDoc( }); } +const byParentDoc = (left: ReadDocMetadata, right: ReadDocMetadata) => { + return (right.data.parentDoc ? 1 : 0) - (left.data.parentDoc ? 1 : 0); +}; + +function sortFiles(filePaths: string[]): ReadDocMetadata[] { + const files = filePaths.map(readDoc).sort(byParentDoc); + const filesBySlug = files.reduce>((bySlug, obj) => { + // eslint-disable-next-line no-param-reassign + bySlug[obj.slug] = obj; + return bySlug; + }, {}); + const dependencies = Object.values(filesBySlug).reduce<[ReadDocMetadata, ReadDocMetadata][]>((edges, obj) => { + if (obj.data.parentDocSlug && filesBySlug[obj.data.parentDocSlug as string]) { + edges.push([filesBySlug[obj.data.parentDocSlug as string], filesBySlug[obj.slug]]); + } + + return edges; + }, []); + + return toposort.array(files, dependencies); +} + /** * Takes a path (either to a directory of files or to a single file) * and syncs those (either via POST or PUT) to ReadMe. @@ -178,11 +203,18 @@ export default async function syncDocsPath( ), ); } + let sortedFiles; + + try { + sortedFiles = sortFiles(files); + } catch (e) { + return Promise.reject(e); + } output = ( await Promise.all( - files.map(async filename => { - return pushDoc(key, selectedVersion, dryRun, filename, cmdType); + sortedFiles.map(async fileData => { + return pushDoc(key, selectedVersion, dryRun, fileData, cmdType); }), ) ).join('\n'); @@ -197,7 +229,9 @@ export default async function syncDocsPath( ), ); } - output = await pushDoc(key, selectedVersion, dryRun, pathInput, cmdType); + + const fileData = readDoc(pathInput); + output = await pushDoc(key, selectedVersion, dryRun, fileData, cmdType); } return Promise.resolve(chalk.green(output)); }