From 152e548bde9e2acc214cec6660350fd4033baf25 Mon Sep 17 00:00:00 2001 From: Glen Whitney Date: Thu, 1 Sep 2022 15:03:20 -0400 Subject: [PATCH] feat(manifest): preserve comments in JSON5 manifests Use npm package `strip-comments-strings` to find all comments in any manifest file as it is read. Save them as part of the "file formatting" detected byt the manifest reader, noting for each the text of the lines they are on, before, and after, and the line number as a fallback. When the manifest is written, attempt to place each comment back in the resulting JSON5 text, so that the text of the line it is on, before, or after (in that priority order) matches the text at time of reading. Otherwise, so that no comments are lost, replace the comment on the same line number, adding a notation that it may have been relocated (due to sorting dependencies, for example, it may no longer be in the same "logical position" in the file, even though it is on the same physical line number). When comments are in fairly ordinary positions and the manifest does not change too drastically (i.e. once the dependencies are sorted as pnpm prefers, and there are not many parameters added all at once), this strategy results in exact preservation of the comments, as a new test shows. The motivation is that one key feature of JSON5 is that it allows human- readable comments, but for this feature to be useful in the case of package.json5 manifests, those comments must be preserved across manifest changes. Partially resolves #2008. [That issue requests this feature also for YAML manifests, but I have no experience working with YAML, and it also requests that key order is preserved, but I did not address this because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly quite deliberately -- so I did not want to simply remove code that appeared to have been purposefully.] --- .../commented-package-json5/modified.json5 | 9 ++ .../commented-package-json5/package.json5 | 9 ++ packages/read-project-manifest/package.json | 3 +- packages/read-project-manifest/src/index.ts | 53 +++++++++- .../types/strip-comments-strings/index.d.ts | 2 + packages/read-project-manifest/test/index.ts | 20 ++++ packages/write-project-manifest/src/index.ts | 97 ++++++++++++++++++- pnpm-lock.yaml | 7 ++ 8 files changed, 195 insertions(+), 5 deletions(-) create mode 100644 packages/read-project-manifest/fixtures/commented-package-json5/modified.json5 create mode 100644 packages/read-project-manifest/fixtures/commented-package-json5/package.json5 create mode 100644 packages/read-project-manifest/src/types/strip-comments-strings/index.d.ts diff --git a/packages/read-project-manifest/fixtures/commented-package-json5/modified.json5 b/packages/read-project-manifest/fixtures/commented-package-json5/modified.json5 new file mode 100644 index 00000000000..6aaa08ea229 --- /dev/null +++ b/packages/read-project-manifest/fixtures/commented-package-json5/modified.json5 @@ -0,0 +1,9 @@ +/* This is an example of a package.json5 file with comments. */ +{ + /* pnpm should keep comments at the same indentation level */ + name: 'foo', + version: '1.0.0', // it should keep in-line comments on the same line + // It should allow in-line comments with no other content + type: 'commonjs', +} +/* And it should preserve comments at the end of the file. Note no newline. */ \ No newline at end of file diff --git a/packages/read-project-manifest/fixtures/commented-package-json5/package.json5 b/packages/read-project-manifest/fixtures/commented-package-json5/package.json5 new file mode 100644 index 00000000000..c6b65fc12af --- /dev/null +++ b/packages/read-project-manifest/fixtures/commented-package-json5/package.json5 @@ -0,0 +1,9 @@ +/* This is an example of a package.json5 file with comments. */ +{ + /* pnpm should keep comments at the same indentation level */ + name: 'foo', + version: '1.0.0', // it should keep in-line comments on the same line + // It should allow in-line comments with no other content + type: 'module', +} +/* And it should preserve comments at the end of the file. Note no newline. */ \ No newline at end of file diff --git a/packages/read-project-manifest/package.json b/packages/read-project-manifest/package.json index 4a074e53e91..8f5d0128696 100644 --- a/packages/read-project-manifest/package.json +++ b/packages/read-project-manifest/package.json @@ -40,7 +40,8 @@ "parse-json": "^5.2.0", "read-yaml-file": "^2.1.0", "sort-keys": "^4.2.0", - "strip-bom": "^4.0.0" + "strip-bom": "^4.0.0", + "strip-comments-strings": "1.2.0" }, "devDependencies": { "@pnpm/read-project-manifest": "workspace:*", diff --git a/packages/read-project-manifest/src/index.ts b/packages/read-project-manifest/src/index.ts index 12d797032d4..e76c8b7a6e6 100644 --- a/packages/read-project-manifest/src/index.ts +++ b/packages/read-project-manifest/src/index.ts @@ -2,10 +2,11 @@ import { promises as fs, Stats } from 'fs' import path from 'path' import PnpmError from '@pnpm/error' import { ProjectManifest } from '@pnpm/types' -import writeProjectManifest from '@pnpm/write-project-manifest' +import { writeProjectManifest, CommentSpecifier } from '@pnpm/write-project-manifest' import readYamlFile from 'read-yaml-file' import detectIndent from 'detect-indent' +import { parseString, stripComments } from 'strip-comments-strings' import equal from 'fast-deep-equal' import isWindows from 'is-windows' import sortKeys from 'sort-keys' @@ -118,9 +119,55 @@ export async function tryReadProjectManifest (projectDir: string): Promise<{ } function detectFileFormatting (text: string) { + const finalNewline = text.endsWith('\n') + if (!finalNewline) { + /* For the sake of the comment parser, which otherwise loses the + * final character of a final comment + */ + text += '\n' + } + const { comments } = parseString(text) + let stripped = stripComments(text) + if (!finalNewline) { + stripped = stripped.slice(0, -1) + } + let offset = 0 // accumulates difference of indices from text to stripped + for (const comment of comments) { + // Unfortunately, JavaScript lastIndexOf does not have an end parameter: + const preamble: string = stripped.slice(0, comment.index - offset) + const lineStart = Math.max(preamble.lastIndexOf('\n'), 0) + const priorLines = preamble.split('\n') + comment.lineNumber = priorLines.length + if (comment.lineNumber === 1) { + if (preamble.trim().length === 0) { + comment.lineNumber = 0 + } + } else { + comment.after = priorLines[comment.lineNumber - 2] + if (priorLines[0].trim().length === 0) { + /* JSON5.stringify will not have a whitespace-only line at the start */ + comment.lineNumber -= 1 + } + } + let lineEnd: number = stripped.indexOf( + '\n', (lineStart === 0) ? 0 : lineStart + 1) + if (lineEnd < 0) { + lineEnd = stripped.length + } + comment.on = stripped.slice(lineStart, lineEnd) + comment.whitespace = stripped + .slice(lineStart, comment.index - offset) + .match(/^\s*/)[0] + const nextLineEnd = stripped.indexOf('\n', lineEnd + 1) + if (nextLineEnd >= 0) { + comment.before = stripped.slice(lineEnd, nextLineEnd) + } + offset += comment.indexEnd - comment.index + } return { + comments, indent: detectIndent(text).indent, - insertFinalNewline: text.endsWith('\n'), + insertFinalNewline: finalNewline, } } @@ -174,6 +221,7 @@ async function readPackageYaml (filePath: string) { function createManifestWriter ( opts: { initialManifest: ProjectManifest + comments?: CommentSpecifier[] indent?: string | number | undefined insertFinalNewline?: boolean manifestPath: string @@ -184,6 +232,7 @@ function createManifestWriter ( updatedManifest = normalize(updatedManifest) if (force === true || !equal(initialManifest, updatedManifest)) { await writeProjectManifest(opts.manifestPath, updatedManifest, { + comments: opts.comments, indent: opts.indent, insertFinalNewline: opts.insertFinalNewline, }) diff --git a/packages/read-project-manifest/src/types/strip-comments-strings/index.d.ts b/packages/read-project-manifest/src/types/strip-comments-strings/index.d.ts new file mode 100644 index 00000000000..5fda8942278 --- /dev/null +++ b/packages/read-project-manifest/src/types/strip-comments-strings/index.d.ts @@ -0,0 +1,2 @@ +// strip-comments-strings/index.d.ts +declare module 'strip-comments-strings'; diff --git a/packages/read-project-manifest/test/index.ts b/packages/read-project-manifest/test/index.ts index fb6a296a569..6e4fa186abd 100644 --- a/packages/read-project-manifest/test/index.ts +++ b/packages/read-project-manifest/test/index.ts @@ -82,6 +82,26 @@ test('preserve space indentation in json5 file', async () => { expect(rawManifest).toBe("{\n name: 'foo',\n dependencies: {\n bar: '1.0.0',\n },\n}\n") }) +test('preserve comments in json5 file', async () => { + const originalManifest = await fs.readFile( + path.join(fixtures, 'commented-package-json5/package.json5'), 'utf8') + const modifiedManifest = await fs.readFile( + path.join(fixtures, 'commented-package-json5/modified.json5'), 'utf8') + + process.chdir(tempy.directory()) + await fs.writeFile('package.json5', originalManifest, 'utf8') + + const { manifest, writeProjectManifest } = await readProjectManifest(process.cwd()) + + // Have to make a change to get it to write anything: + const newManifest = Object.assign({}, manifest, { type: 'commonjs' }) + + await writeProjectManifest(newManifest) + + const resultingManifest = await fs.readFile('package.json5', 'utf8') + expect(resultingManifest).toBe(modifiedManifest) +}) + test('do not save manifest if it had no changes', async () => { process.chdir(tempy.directory()) diff --git a/packages/write-project-manifest/src/index.ts b/packages/write-project-manifest/src/index.ts index c4964b0826f..a5d0ea40ac4 100644 --- a/packages/write-project-manifest/src/index.ts +++ b/packages/write-project-manifest/src/index.ts @@ -10,10 +10,21 @@ const YAML_FORMAT = { noRefs: true, } -export default async function writeProjectManifest ( +export interface CommentSpecifier { + type: string + content: string + lineNumber: number + after?: string + on: string + whitespace: string + before?: string +} + +export async function writeProjectManifest ( filePath: string, manifest: ProjectManifest, opts?: { + comments?: CommentSpecifier[] indent?: string | number | undefined insertFinalNewline?: boolean } @@ -26,8 +37,90 @@ export default async function writeProjectManifest ( await fs.mkdir(path.dirname(filePath), { recursive: true }) const trailingNewline = opts?.insertFinalNewline === false ? '' : '\n' - const json = (fileType === 'json5' ? JSON5 : JSON) + let json = (fileType === 'json5' ? JSON5 : JSON) .stringify(manifest, undefined, opts?.indent ?? '\t') + if (opts?.comments) { + // We need to reintroduce the comments. So create an index of + // the lines of the manifest so we can try to match them up. + // We eliminate whitespace and quotes because pnpm may have changed them. + const jsonLines = json.split('\n') + const index = {} + for (let i = 0; i < jsonLines.length; ++i) { + const key = jsonLines[i].replace(/[\s'"]/g, '') + if (key in index) { + index[key] = -1 + } else { + index[key] = i + } + } + + // A place to put comments that come _before_ the lines they are + // anchored to: + const jsonPrefix: Record = {} + for (const comment of opts.comments) { + // First if we can find the line the comment was on, that is + // the most reliable locator: + let key = comment.on.replace(/[\s'"]/g, '') + if (key && index[key] !== undefined && index[key] >= 0) { + jsonLines[index[key]] += ' ' + comment.content + continue + } + // Next, if it's not before anything, it must have been at the very end: + if (comment.before === undefined) { + jsonLines[jsonLines.length - 1] += comment.whitespace + comment.content + continue + } + // Next, try to put it before something; note the comment extractor + // used the convention that position 0 is before the first line: + let location = (comment.lineNumber === 0) ? 0 : -1 + if (location < 0) { + key = comment.before.replace(/[\s'"]/g, '') + if (key && index[key] !== undefined) { + location = index[key] + } + } + if (location >= 0) { + if (jsonPrefix[location]) { + jsonPrefix[location] += ' ' + comment.content + } else { + const inlineWhitespace = comment.whitespace.startsWith('\n') + ? comment.whitespace.slice(1) + : comment.whitespace + jsonPrefix[location] = inlineWhitespace + comment.content + } + continue + } + // The last definite indicator we can use is that it is after something: + if (comment.after) { + key = comment.after.replace(/[\s'"]/g, '') + if (key && index[key] !== undefined && index[key] >= 0) { + jsonLines[index[key]] += comment.whitespace + comment.content + continue + } + } + // Finally, try to get it in the right general location by using the + // line number, but warn the user the comment may have been relocated: + location = comment.lineNumber - 1 // 0 was handled above + let separator = ' ' + if (location >= jsonLines.length) { + location = jsonLines.length - 1 + separator = '\n' + } + jsonLines[location] += separator + comment.content + + ' /* [comment possibly relocated by pnpm] */' + } + // Insert the accumulated prefixes: + for (let i = 0; i < jsonLines.length; ++i) { + if (jsonPrefix[i]) { + jsonLines[i] = jsonPrefix[i] + '\n' + jsonLines[i] + } + } + // And reassemble the manifest: + json = jsonLines.join('\n') + } + return writeFileAtomic(filePath, `${json}${trailingNewline}`) } + +export default writeProjectManifest diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 478d75b049d..d403437be9c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4587,6 +4587,9 @@ importers: strip-bom: specifier: ^4.0.0 version: 4.0.0 + strip-comments-strings: + specifier: 1.2.0 + version: 1.2.0 devDependencies: '@pnpm/read-project-manifest': specifier: workspace:* @@ -14815,6 +14818,10 @@ packages: resolution: {integrity: sha512-3xurFv5tEgii33Zi8Jtp55wEIILR9eh34FAW00PZf+JnSsTmV/ioewSgQl97JHvgjoRGwPShsWm+IdrxB35d0w==} engines: {node: '>=8'} + /strip-comments-strings/1.2.0: + resolution: {integrity: sha512-zwF4bmnyEjZwRhaak9jUWNxc0DoeKBJ7lwSN/LEc8dQXZcUFG6auaaTQJokQWXopLdM3iTx01nQT8E4aL29DAQ==} + dev: false + /strip-final-newline/2.0.0: resolution: {integrity: sha512-BrpvfNAE3dcvq7ll3xVumzjKjZQ5tI1sEUIKr3Uoks0XUl45St3FlatVqef9prk4jRDzhW6WZg+3bk93y6pLjA==} engines: {node: '>=6'}