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
Request: Preserve comments and key orders in package.yaml when run 'pnpm install --save', 'pnpm update --latest', etc. #2008
Comments
@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. |
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 pnpm#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.]
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. |
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 sindresorhus/detect-indent#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. |
@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. |
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 by 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 for this commit is to take advantage of the feature of JSON5 that it allows human-readable comments. For this feature to be useful in the case of package.json5 manifests, those comments must be preserved across manifest changes. Partially resolves pnpm#2008. [That issue requests comment preservation also for YAML manifests, but I have no experience working with YAML, and it also requests that key order be preserved, but I did not address key order because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly deliberately -- so I did not want to simply remove code that appeared to have been purposefully written and included.]
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 by 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 for this commit is to take advantage of the feature of JSON5 that it allows human-readable comments. For this feature to be useful in the case of package.json5 manifests, those comments must be preserved across manifest changes. Partially resolves pnpm#2008. [That issue requests comment preservation also for YAML manifests, but I have no experience working with YAML, and it also requests that key order be preserved, but I did not address key order because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly deliberately -- so I did not want to simply remove code that appeared to have been purposefully written and included.]
OK, now that we have gotten past the former bugs in |
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 by 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 for this commit is to take advantage of the feature of JSON5 that it allows human-readable comments. For this feature to be useful in the case of package.json5 manifests, those comments must be preserved across manifest changes. Partially resolves pnpm#2008. [That issue requests comment preservation also for YAML manifests, but I have no experience working with YAML, and it also requests that key order be preserved, but I did not address key order because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly deliberately -- so I did not want to simply remove code that appeared to have been purposefully written and included.]
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 by 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 for this commit is to take advantage of the feature of JSON5 that it allows human-readable comments. 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 comment preservation also for YAML manifests, but I have no experience working with YAML, and it also requests that key order be preserved, but I did not address key order because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly deliberately -- so I did not want to simply remove code that appeared to have been purposefully written and included.] Co-authored-by: Zoltan Kochan <z@kochan.io>
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. |
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. |
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.) |
@KSXGitHub @zkochan @gwhitney @gusbemacbe @evidolob @mohitsuman @msivasubramaniaan @mptr 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
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: # 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
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: # 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
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: # 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… |
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 |
Following on from pnpm#2008 (comment) … Closes pnpm#2008.
@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. |
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) |
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. |
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
:will be turn into this:
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 beforedependencies
anddevDependencies
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:
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.
The text was updated successfully, but these errors were encountered: