feat(pkg-manifest): preserve comments in YAML manifests #6273
Conversation
Following on from pnpm#2008 (comment) … Closes pnpm#2008.
|
💖 Thanks for opening this pull request! 💖 |
| @@ -1,9 +1,24 @@ | |||
| import gfs from '@pnpm/graceful-fs' | |||
| import { type ProjectManifest } from '@pnpm/types' | |||
| import YAML from 'yaml' | |||
There was a problem hiding this comment.
we use js-yaml
we shouldn't use 2 libraries for parsing yaml.
There was a problem hiding this comment.
@danielbayley, can you update the suggestion given by @zkochan?
I am desperate to preserve the comments in the pakcage.yaml.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.yamlfile betweenyamlvsjs-yaml. - There's patches to
js-yamlthat we'll need to replicate inyaml. 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.
There was a problem hiding this comment.
@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-yamltoyamlthat 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…
There was a problem hiding this comment.
Sounds good! Thanks @danielbayley.
|
@zkochan, do you have to review this PR and to test, modifying a bit, to see if it will work? We are desperate to preserve the comments in the |
I think Zoltan's last review around avoiding 2 yaml libraries makes sense and still applies today. Would be curious for his thoughts on whether we should migrate in a separate PR though. |
Following on from the basic implementation outlined here in #2008—which works well—am I right in thinking that the
manifest: datahere (from here) containing the AST (including comments) getsnormalized, and therefore lost here?@zkochan I’m keen to get this feature merged, but could do with some pointers as to the best approach…
Closes #2008.