Skip to content
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

package.json - EOF newline removed #15

Closed
wiese opened this issue Oct 14, 2020 · 8 comments · Fixed by #19
Closed

package.json - EOF newline removed #15

wiese opened this issue Oct 14, 2020 · 8 comments · Fixed by #19

Comments

@wiese
Copy link
Contributor

wiese commented Oct 14, 2020

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. npm/npm#18545 or phetsims/perennial#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.

wiese added a commit to wmde/wikit that referenced this issue Oct 14, 2020
This was created with the help of lerna-audit and manually reverting the
changes created to the package.json[1].

[1]: tnobody/lerna-audit#15
wiese added a commit to wmde/wikit that referenced this issue Oct 14, 2020
This was created with the help of lerna-audit and manually reverting the
changes created to the package.json[1].

[1]: tnobody/lerna-audit#15
Ladsgroup pushed a commit to wmde/wikit that referenced this issue Oct 15, 2020
This was created with the help of lerna-audit and manually reverting the
changes created to the package.json[1].

[1]: tnobody/lerna-audit#15
jakobw pushed a commit to wmde/wikit that referenced this issue Oct 15, 2020
This was created with the help of lerna-audit and manually reverting the
changes created to the package.json[1].

[1]: tnobody/lerna-audit#15
jakobw pushed a commit to wmde/wikit that referenced this issue Oct 15, 2020
This was created with the help of lerna-audit and manually reverting the
changes created to the package.json[1].

[1]: tnobody/lerna-audit#15
@brandonb927
Copy link

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
Copy link
Collaborator

svettwer commented Feb 9, 2021

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
Copy link

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
Copy link
Collaborator

svettwer commented Feb 10, 2021

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

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
Copy link
Contributor Author

wiese commented Feb 24, 2021

@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
Copy link

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

@brandonb927
Copy link

brandonb927 commented Feb 25, 2021

@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
Copy link
Contributor Author

wiese commented Mar 2, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants