Skip to content
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

feat(pkg-manifest): preserve comments in YAML manifests #6273

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,9 @@
# This is an example of a package.yaml 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.
@@ -0,0 +1,9 @@
# This is an example of a package.yaml 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.
22 changes: 15 additions & 7 deletions pkg-manifest/read-project-manifest/src/index.ts
Expand Up @@ -4,12 +4,12 @@ import { PnpmError } from '@pnpm/error'
import { type ProjectManifest } from '@pnpm/types'
import { extractComments, type CommentSpecifier } from '@pnpm/text.comments-parser'
import { writeProjectManifest } from '@pnpm/write-project-manifest'
import readYamlFile from 'read-yaml-file'
import detectIndent from '@gwhitney/detect-indent'
import equal from 'fast-deep-equal'
import isWindows from 'is-windows'
import sortKeys from 'sort-keys'
import {
readYamlFile,
readJson5File,
readJsonFile,
} from './readFile'
Expand Down Expand Up @@ -86,11 +86,15 @@ export async function tryReadProjectManifest (projectDir: string): Promise<{
}
try {
const manifestPath = path.join(projectDir, 'package.yaml')
const manifest = await readPackageYaml(manifestPath)
const { data, text } = await readPackageYaml(manifestPath)
return {
fileName: 'package.yaml',
manifest,
writeProjectManifest: createManifestWriter({ initialManifest: manifest, manifestPath }),
manifest: data,
writeProjectManifest: createManifestWriter({ //initialManifest: data, manifestPath }),
...detectFileFormatting(text), //AndComments(text),
initialManifest: data,
manifestPath,
})
}
} catch (err: any) { // eslint-disable-line
if (err.code !== 'ENOENT') throw err
Expand Down Expand Up @@ -160,10 +164,14 @@ export async function readExactProjectManifest (manifestPath: string) {
}
}
case 'package.yaml': {
const manifest = await readPackageYaml(manifestPath)
const { data, text } = await readPackageYaml(manifestPath)
return {
manifest,
writeProjectManifest: createManifestWriter({ initialManifest: manifest, manifestPath }),
manifest: data,
writeProjectManifest: createManifestWriter({ //initialManifest: manifest, manifestPath }),
...detectFileFormatting(text), //AndComments(text),
initialManifest: data,
manifestPath,
}),
}
}
}
Expand Down
15 changes: 15 additions & 0 deletions pkg-manifest/read-project-manifest/src/readFile.ts
@@ -1,9 +1,24 @@
import gfs from '@pnpm/graceful-fs'
import { type ProjectManifest } from '@pnpm/types'
import YAML from 'yaml'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use js-yaml
we shouldn't use 2 libraries for parsing yaml.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbayley, can you update the suggestion given by @zkochan?

I am desperate to preserve the comments in the pakcage.yaml.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use js-yaml
we shouldn't use 2 libraries for parsing yaml.

@zkochan I agree that 2 is not ideal… I suppose I’m proposing that we replace js-yaml with eemeli.org/yaml, because it retains comments very well, whereas js-yaml does not, and instead just discards them. Did you take a look at my working implementation here? #2008 (comment)

can you update the suggestion given by @zkochan?

@gusbemacbe Unfortunately it isn’t a suggestion that helps with this implementation…

I am desperate to preserve the comments in the *package.yaml.

Same!

Copy link
Member

@gluxon gluxon Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a bit more research, I'd also be in favor of a move to yaml. It looks like js-yaml hasn't had a release for a few years.

Switching to yaml to avoid 2 libraries for parsing/serializing YAML files will take a bit of work though.

  • Is there a performance difference? It'll be interesting to see a performance benchmark on serializing/deserializing a large pnpm-lock.yaml file between yaml vs js-yaml.
  • There's patches to js-yaml that we'll need to replicate in yaml. nodeca/js-yaml@master...pnpm:js-yaml:features-for-pnpm

@danielbayley Do you happen to have bandwidth for such an investigation? If not, we can spin up a different pull request investigating a switch from js-yaml to yaml that would merge before this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbayley Do you happen to have bandwidth for such an investigation?

Unfortunately not, currently.

If not, we can spin up a different pull request investigating a switch from js-yaml to yaml that would merge before this PR.

@gluxon Fully in favour of the switch… and definitely agree that should be a separate PR, preceding this one. I would be keen to pick up the baton after that to implement comment preservation…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Thanks @danielbayley.

import JSON5 from 'json5'
import parseJson from 'parse-json'
import stripBom from 'strip-bom'

export async function readYamlFile (filePath: string) {
const text = await readFileWithoutBom(filePath)
try {
return {
data: YAML.parseDocument(text),
text,
}
} catch (err: any) { // eslint-disable-line
err.message = `${err.message as string} in ${filePath}`
err['code'] = 'ERR_PNPM_YAML_PARSE'
throw err
}
}

export async function readJson5File (filePath: string) {
const text = await readFileWithoutBom(filePath)
try {
Expand Down
20 changes: 20 additions & 0 deletions pkg-manifest/read-project-manifest/test/index.ts
Expand Up @@ -101,6 +101,26 @@ test('preserve comments in json5 file', async () => {
const resultingManifest = await fs.readFile('package.json5', 'utf8')
expect(resultingManifest).toBe(modifiedManifest)
})
/* FIXME
test('preserve comments in YAML file', async () => {
const originalManifest = await fs.readFile(
path.join(fixtures, 'commented-package-yaml/package.yaml'), 'utf8')
const modifiedManifest = await fs.readFile(
path.join(fixtures, 'commented-package-yaml/modified.yaml'), 'utf8')

process.chdir(tempy.directory())
await fs.writeFile('package.yaml', 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.yaml', 'utf8')
expect(resultingManifest).toBe(modifiedManifest)
})*/

test('do not save manifest if it had no changes', async () => {
process.chdir(tempy.directory())
Expand Down
14 changes: 9 additions & 5 deletions pkg-manifest/write-project-manifest/src/index.ts
Expand Up @@ -2,13 +2,16 @@ import { promises as fs } from 'fs'
import path from 'path'
import { insertComments, type CommentSpecifier } from '@pnpm/text.comments-parser'
import { type ProjectManifest } from '@pnpm/types'
import YAML from 'yaml'
import JSON5 from 'json5'
import writeFileAtomic from 'write-file-atomic'
import writeYamlFile from 'write-yaml-file'

const YAML_FORMAT = {
noCompatMode: true,
noRefs: true,
//noCompatMode: true, // TODO don't try to be compatible with older yaml versions
//noRefs: true, // TODO don't convert duplicate objects into references
//indent: 2,
//indentSeq: false,
//singleQuote: false,
}

export async function writeProjectManifest (
Expand All @@ -20,12 +23,13 @@ export async function writeProjectManifest (
insertFinalNewline?: boolean
}
): Promise<void> {
await fs.mkdir(path.dirname(filePath), { recursive: true })
const fileType = filePath.slice(filePath.lastIndexOf('.') + 1).toLowerCase()
if (fileType === 'yaml') {
return writeYamlFile(filePath, manifest, YAML_FORMAT)
const yaml = YAML.stringify(manifest, YAML_FORMAT)
return writeFileAtomic(filePath, yaml)
}

await fs.mkdir(path.dirname(filePath), { recursive: true })
const trailingNewline = opts?.insertFinalNewline === false ? '' : '\n'
const indent = opts?.indent ?? '\t'

Expand Down