KSXGitHub commented 4 years ago

Problem

One of the advantages of format such as YAML and JSON5 is that it supports comments. Commands that modifies these files render these comments pointless.

For example, this package.yaml:

dependencies:

  # the following dependencies are required to do something
  foo: 0.1.2
  bar: 1.2.3

  # the following dependencies are others' peer
  abc: 2.3.4 # peer dependency of foo
  def: 3.4.5 # peer dependency of bar

will be turn into this:

dependencies:
  abc: 2.3.4
  bar: 1.2.3
  def: 3.4.5
  foo: 0.1.2

Suggestion

1. Attempt on a subset of YAML first

YAML is a complicated format, as such, it is understandable to use libraries such as js-yaml to write a YAML files. However, most of the time, YAML that users write is rather simple: No leading indent before dependencies and devDependencies field, one level of indent before each dependency name. For this reason, one can remove/insert lines with correct indentation in between YAML content and get the desired result.

Possible strategy:

Check if the YAML string satisfies a strict subset (such as No JSON block).
If it satisfies, treat YAML like a list of lines.
If it doesn't, and if user allows it, use YAML library to deserialize object.

2. Do not edit YAML file by default

pnpm may choose to ignore package.yaml file and print a warning, or pnpm may emit an error, depends on the command.

3. New command-line flags

When YAML file satisfies the subset

pnpm install --append-yaml-lines: Add new lines to the end of dependencies/devDependencies list.
pnpm install --prepend-yaml-lines: Add new lines to the beginning of dependencies/devDependencies list.
pnpm install --under-yaml-comment [number|string]: Add new lines to under a comment block.

When YAML file does not satisfy the subset:

pnpm <command> --[no-]force-dump-yaml: Whether to use YAML library to dump YAML content.

This may also apply to JSON5

JSON5 has the following features: Comments, trailing commas, etc.

danielbayley commented 2 years ago

@KSXGitHub @zkochan Maybe something like yaml-ast-parser or yawn-yaml would be useful here?

Also, for reference: https://stackoverflow.com/a/62854961/7307976 and #5677.

gusbemacbe commented 2 years ago

Chao buoi toi @KSXGitHub and dobryi vechir @zkochan!

@gwhitney has just created a new feature to preservation of comments, but only for JSON5. As he can't help with YAML, maybe Red Hat developers, who developed a VSCode extension to format YAML, can help a bit.

Please forgive me for pinging you. I think you know doing it, and perhaps you can help.

@evidolob
@mohitsuman
@msivasubramaniaan

gwhitney commented 2 years ago

Wow, yes, I haven't even submitted that as a PR yet. Thanks for your interest in it. I think it's pretty much ready, but there is one hitch, that detect-indent has a major bug in it (it does not implement its documented algorithm, see https://github.com/sindresorhus/detect-indent/pull/34) and so it disrupts the indentation of all of my commented package.json5 files. So I was waiting for that bug to be fixed on detect-indent and published, so that first I could submit a PR to pnpm updating to that new version of detect-indent once it becomes available, and then I could submit the comment-preservation change PR after that.

As long as we are on the topic, though, I do have to be clear that the strategy of my comment-preserving commit for pnpm is unfortunately not to switch to a parser that attaches the comments to nodes of the resulting JSON5 data (not quite sure how that would work in terms of being transparent to users of the data itself, though) but rather to remember the comments as part of the "formatting" of the JSON5, noting the text of the lines that the comments are on, before, and after, and then restoring them in the regenerated file to preserve those things, in that priority order. There are various edge cases this doesn't handle, in which case the commit puts the comment back as near to its original line number as it can, with a notation that it might have been moved (so no information is lost). But in practice on my package.json5 files commented in "ordinary" ways (front matter comments at the top, comments explaining the usage of scripts occurring before the scripts, and in-line comments on entries in dependencies explaining why they are there), the comments appear nicely invariant. So it is a "practical" solution, rather than a theoretically crisp solution, So at some point we will need a judgment from the architect (@zkochan, I presume) whether pnpm is willing to go ahead with such an approach.

mohitsuman commented 2 years ago

@gusbemacbe Thanks for tagging me around this. I am the PM for the VSCode YAML and would discuss with the team to see the possible scenarios around this if possible.

gwhitney commented 1 year ago

OK, now that we have gotten past the former bugs in detect-indent, I have rebased the code to preserve comments in json5 manifests and submitted it as a pull request. It is working well for me in real-world projects using json5 manifests. Sorry I don't really have any idea how to address the same issue in yaml. Hopefully zkochan will be OK with the pragmatic approach of remembering the comments as part of the "formatting" and re-inserting them when writing out the manifest, just as is done with the indentation, for example.

gwhitney commented 1 year ago

Sorry I don't really think I can help with yaml, but thanks for merging the comment-preserving for JSON5. Could you clarify about whether you want key orders to be preserved in JSON5 (in which case I am happy to do a PR), or prefer them to be deliberately modified (i.e., made predictable by sorting as they are now, and as the code was clearly written to do)? I assume the latter, but just thought it should be clarified here so that we know what remains to be done to complete this issue. Thanks again.

zkochan commented 1 year ago

The keys are currently sorted because that is what npm also does. And I think it is probably a good idea to sort the keys as it reduces possible merge conflicts.

gwhitney commented 1 year ago

That seems totally fine to me. So I guess that leaves just preserving comments in yaml package manifests, as far as this issue goes. (So you might want to remove or strike out "and key orders" in the issue title.)

danielbayley commented 1 year ago

@KSXGitHub @zkochan @gwhitney @gusbemacbe @evidolob @mohitsuman @msivasubramaniaan @mptr So looking into this further, this seems to be doable with any of the following 3 approaches…

Given source YAML:

# package.yaml
name: example
version: 0.1.0
keywords:
- test

packageManager: ^pnpm@7.30.0

# comment
dependencies:
  js-yaml: ^4.1.0

scripts: # inline comment
  test: echo $npm_command

with mohsen1/yawn-yaml


const YAWN = require("yawn-yaml/cjs") // import not working…
const fs = require("node:fs")

const manifest = "package.yaml"

const add = "yawn-yaml" const version = "^1.5.0"

const yaml = fs.readFileSync(manifest, "utf8") const yawn = new YAWN(yaml) let {dependencies} = yawn.json

dependencies[add] = version

yawn.json = { ...yawn.json, dependencies }

console.log(yawn.yaml)

gives:
~~~ yaml
# package.yaml
name: example
version: 0.1.0
keywords:
- test

packageManager: ^pnpm@7.30.0

# comment
dependencies:
  js-yaml: ^4.1.0
  yawn-yaml: ^1.5.0

scripts: # inline comment
  test: echo $npm_command

with CodeLenny/yawn-yaml-cli:


import {exec} from "node:child_process"

const manifest = "package.yaml"

const add = "yawn-yaml-cli" const version = "^1.0.1"

npx yawn-yaml-cli

exec(yawn set ${manifest} dependencies.${add} ${version})

gives:
~~~ yaml
# package.yaml
name: example
version: 0.1.0
keywords:
- test

packageManager: ^pnpm@7.30.0

# comment
dependencies:
  js-yaml: ^4.1.0
  yawn-yaml-cli: ^1.0.1

scripts: # inline comment
  test: echo $npm_command

with eemeli/yaml (repo here):


import fs from "node:fs/promises"
import { parseDocument, stringify } from "yaml"

const manifest = "package.yaml"

const add = "yaml" const version = "^2.2.1"

const doc = parseDocument(await fs.readFile(manifest, "utf8"))

const dependencies = doc.get("dependencies") let dependency = doc.createPair(add, version) dependencies.add(dependency)

const yaml = stringify(doc) console.log(yaml)

gives:
~~~ yaml
# package.yaml
name: example
version: 0.1.0
keywords:
  - test

packageManager: ^pnpm@7.30.0

# comment
dependencies:
  js-yaml: ^4.1.0
  yaml: ^2.2.1

scripts:
  # inline comment
  test: echo $npm_command

3 (eemeli/yaml) looks like the most mature/maintained library for this (81 forks, 866 stars, latest commit only last week…

danielbayley commented 1 year ago

Just throwing up this basic implementation before I work it into a PR:

import { readFile } from "node:fs/promises"
import { parseDocument, stringify } from "yaml"

const manifest = "package.yaml"
let flag = "--save-dev" // devDependencies
const add = "yaml"
const version = "^2.2.1"

let options = {
  indent: 2, // default
  indentSeq: false, // default: true
  sort: true, // default
}

flag ??= "--save-prod" // default

const dependencies = {
  devDependencies: ["--save-dev", "-D"],
  dependencies: ["--save-prod", "-P"], //, "--save", "-S"],
  peerDependencies: "--save-peer",
  optionalDependencies: ["--save-optional", "-O"],
  bundleDependencies: ["--save-bundle", "-B"],
}

const metadata = await readFile(manifest, "utf8")
const doc = parseDocument(metadata, options)

const invert = object =>
  Object.entries(object).reduce((obj, [key, value]) => {
    [value].flat().forEach(v => (obj[v] = key))
    return obj
  }, {})

const save = invert(dependencies)[flag]
let dependency = doc.createPair(add, version)
doc.addIn([save], dependency)

if (options.sort) {
  const sort = Object.keys(dependencies)
  doc.contents.items = doc.contents.items.map(item => {
    if (sort.includes(item.key?.value)) item.value.items = item.value.items.sort()
    return item
  })
}

const yaml = stringify(doc, options)
console.log(yaml)

Given:

# package.yaml
name: example
version: 0.1.0
keywords:
- test

packageManager: ^pnpm@7.30.0

devDependencies:
  zx: ^7.2.1 # inline comment

# comment
dependencies:
  js-yaml: ^4.1.0

scripts: # inline comment
  test: echo $npm_command

gives:

# package.yaml
name: example
version: 0.1.0
keywords:
- test

packageManager: ^pnpm@7.30.0

devDependencies:
  yaml: ^2.2.1
  zx: ^7.2.1 # inline comment

# comment
dependencies:
  js-yaml: ^4.1.0

scripts:
  # inline comment
  test: echo $npm_command

@KSXGitHub @zkochan @gwhitney @gusbemacbe @evidolob @mohitsuman @msivasubramaniaan @mptr @jcayzac @alexgorbatchev Feel free to critique… Follow along in #6273.

tobia commented 5 months ago

@zkochan IMO sorting the keys goes against the purpose of allowing comments in the first place.

For example, one of my projects has 100+ dependencies, some of which are only there for a particular legacy reason, or their version is fixed is some way because of a particular bug. If I could separate the 100+ items in sections, each with its own comment and description, then it would make the file much more manageable. But if pnpm will destroy that order every time somebody uses one of the commands to add/remove a package, then it makes no sense to allow comments in the file.

alexgorbatchev commented 5 months ago

I want to +1 the importance of preserving the comments and ordering , this is very important for large enterprise projects with multiple computers for many reasons (history, reasoning, tagging packages that can't be upgraded or should be upgraded at a given time, etc)

gwhitney commented 5 months ago

I can only comment on JSON5, as that's what I implemented, and what I use. Whether to sort keys or preserve their input ordering is up to Zoltan, of course. I just want to remind everyone that the comment preservation currently implemented for JSON5 attempts to keep comments on the nearest line with the same text as they were on, or if the comment was on a line by itself, before the nearest line that matches the text the comment was before when the file was read in, or failing that, after the nearest line that matches the text the comment was after. If all else fails, it puts the comment back at its original line number. This seems to work pretty well in practice to keep comments with the entries in the JSON5 file they are commenting, even when keys move because of sorting.

pnpm / pnpm

Request: Preserve comments and key orders in package.yaml when run 'pnpm install --save', 'pnpm update --latest', etc. #2008

Problem

Suggestion

1. Attempt on a subset of YAML first

2. Do not edit YAML file by default

3. New command-line flags

This may also apply to JSON5

npx yawn-yaml-cli