search

tnobody / lerna-audit

Micro util to run npm audit in lerna monorepos
9 stars 11 forks source link

package.json - EOF newline removed #15

Closed wiese closed 3 years ago

wiese commented 3 years ago

Using lerna-audit (1.2.0) on my package.jsons generated by npm 6.14.5 results in the EOF newline being removed.

Looking at e.g. https://github.com/npm/npm/issues/18545 or https://github.com/phetsims/perennial/issues/156 I don't want to get into the territory if having the newline is "right" or not, but I think it would be in the interest of everyone if changes to those files were confined to version numbers.

$ git diff
──────────────────────────
modified: my-sub-package/package.json
──────────────────────────
@ my-sub-package/package.json:51 @
  }
}

}

\ No newline at end of file

FWIW lerna-audit uses JSON.stringify() to put the package.json back together once it has done its job. Maybe this could become a little more involved, respecting the original presence of a newline - one way or another.

brandonb927 commented 3 years ago

Re-surfacing this because I just got bit by this issue and now I can't use this package because we enforce newlines at the end of files.

svettwer commented 3 years ago

If this package helps you in general but is not usable for you because of this issue, maybe you could propose a PR fixing this. 😁 PRs are highly appreciated for sure.

brandonb927 commented 3 years ago

If this package helps you in general but is not usable for you because of this issue, maybe you could propose a PR fixing this. 😁 PRs are highly appreciated for sure.

I would definitely be interested in supplying a PR to fix the issue, however I simply don't have the time and I can understand as a maintainer you may not either. I wanted to let you know this issue affected more users than just the OP, that's all.

svettwer commented 3 years ago

Okay, thanks! =) It's always helpful to know how important an issue is. How should one prioritize without such information?! :+1:

I simply don't have the time

No Problem. I just like to encourage people to think about proposing a PR. Most times, people opening issues have a much deeper understanding of the issue than the maintainers at least in the first place which makes a fix very easy with that knowledge.

I'll keep in mind that this is affecting more people. And if I understand you correctly, this is a blocker for you. So this is definitely something to take care of.

wiese commented 3 years ago

@brandonb927 Would you dare to try #19 and report back so that I can either tend to the things I missed or we give @svettwer some confidence to go ahead with the PR? Thanks!

brandonb927 commented 3 years ago

@wiese can't guarantee a timeline, but I will keep this tab open as a reminder to try it!

brandonb927 commented 3 years ago

@wiese I installed your branch and tested it, it works great! I think this would be a great addition to this library. On the plus side, it also sorts your dependencies properly as a bonus (probably not intended but a nice side effect).

wiese commented 3 years ago

Thanks for your feedback, @brandonb927! Let's see what @svettwer makes of it.

it also sorts your dependencies

That's definitely a possibility if they were "not ordered" to begin with. They will be ordered exactly the way npm itself would leave them, as its very own implementation is used. It is opinionated but I guess still superior to and less work than defining our own standard and implementing that.