yarnpkg / berry

📦🐈 Active development trunk for Yarn ⚒
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.37k stars 1.1k forks source link

[Bug] Preserve comments and styling in YAML configuration #1463

Open bertho-zero opened 4 years ago

bertho-zero commented 4 years ago

Whenever I update Yarn 2 or I want to test a branch with yarn set version from sources it changes the yarnrc.yml.

The order of the fields changes and the commented lines disappear, is it possible to keep the file as close as possible to the state in which the user left it?

yarnbot commented 4 years ago

Hi! 👋

This issue looks stale, and doesn't feature the reproducible label - which implies that you didn't provide a working reproduction using Sherlock. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it or you edit your first post to include a formal reproduction (you can use the playground for that).

Note that we require Sherlock reproductions for long-lived issues (rather than standalone git repositories or similar) because we're a small team. Sherlock gives us the ability to check which bugs are still affecting the master branch at any given point, and decreases the amount of code we need to run on our own machines (thus leading to faster bug resolutions). It helps us help you! 😃

If you absolutely cannot reproduce a bug on Sherlock (for example because it's a Windows-only issue), a maintainer will have to manually add the upholded label. Thanks for helping us triaging our repository! 🌟

paul-soporan commented 4 years ago

Status update: I've started and mostly finished a project called enhanced-yaml built on top of the yaml library which can modify YAML files while preserving comments and styling with very high accuracy. I've tested it in Yarn and it does its job quite well with .yarnrc.yml files (only a few tweaks remaining).

The biggest problem is that it's based on the yaml library, which, even though it has a powerful AST and great ways to manipulate it, is much slower than js-yaml for parsing and much slower than our custom stringifier for stringifying.

Speed comparison on a simple install on our repository:

With the current speed stats, we can't replace the current parser and also not the current stringifier which is used for the lockfile. Because of this, currently our only option is to keep using js-yaml for the parser, our custom stringifier for the lockfile, and enhanced-yaml for the configuration (the only place where preserving comments and styling matters).

Unfortunately, using this combination would add quite a lot of extra KB to our bundle size (didn't test it with the "all 3" combination, but when replacing js-yaml and our custom stringifier with enhanced-yaml, the bundle size jumped up from 1.72 MB to 1.83 MB). The problem is that yaml is quite a heavy library because of how feature-packed it is (my wrapper library is a 4 KB wrapper in comparison - when minified) and it also can't be easily tree-shakeable because of the heavy use of classes.

Because of this, I'm curious how wanted this feature (fix?) is and if it would be worth it to implement (you can add reactions to this issue). In the meantime, I'll try to (hopefully) optimize the bundle size and see if it would be practical for us to include this.

Note: We'd also have another use for this library. We need a custom duplicate handler for some special lockfile merge conflict cases and I'm intending to implement a custom duplicate handler option inside enhanced-yaml (possible because of yaml's awesome AST, not possible with js-yaml).

Another note: I've also looked into using other YAML libraries, but none of them except yaml have what we need.

borekb commented 4 years ago

@paul-soporan I just came to search the issues after Yarn deleted my comments from .yarnrc.yml file (which made a bit angry to be honest 😄; I hate when tools silently drop my data) so it's great to see your thoughtful comment above.

I'll just say a few things:

belgattitude commented 2 years ago

@paul-soporan

Because of this, I'm curious how wanted this feature (fix?) is and if it would be worth it to implement (you can add reactions to this issue).

Personally I don't feel a huge need, but I saw a psychological effect on teams when migrating or upgrading to yarn 3. Can't explain in details, but I realized comments are helpful for many people. Loosing comments on update is not well perceived and saw "as one more yarn bug"). It's not a big deal to handle, but if a better option exists, why not ?

Status update: I've started and mostly finished a project called enhanced-yaml built on top of the yaml library which can modify YAML

It's a very nice addition indeed. I quickly drafted a P/R to see what yaml@v2.0.0-10 would bring as an overhead. I wasn't aware you tried the same road.

The P/R is there, with some (I hope) helpful insights / findings.

https://github.com/yarnpkg/berry/pull/4135

But the your enhanced-yaml seems more advanced already. And between v1 and v2 the size increase stays valid. Haven't checked if speed is better in v2.

lensbart commented 2 years ago

I'm curious how wanted this feature (fix?) is and if it would be worth it to implement (you can add reactions to this issue).

I think it would be quite helpful to get this fixed. My yarnrc.yml contains useful comments and gets overwritten :

I understand the bundle size concerns, but this does feel like a bug. Maybe as a temporary (and admittedly ugly) band-aid, a warning could be shown that comments have been stripped?

Thank you!

belgattitude commented 2 years ago

@lensbart

The road i tried didn't give what i hoped for.

bundle size is okay, but too slow in my tests

https://github.com/yarnpkg/berry/pull/4135#discussion_r810542028

Another idea welcome

Vages commented 1 year ago

While I agree that the best thing would be to have comments persist in the file, I think Yarn should insert a comment at the top of the file at each edit saying something like:

# Do not add comments to this file.
# They will disappear whenever Yarn makes updates to it.