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

Do not pretty-print package-lock.json to prevent corrupting git lines count history #186

Closed
wants to merge 1 commit into from

Conversation

bl00mber
Copy link
Contributor

@bl00mber bl00mber commented Apr 5, 2019

Because package-lock.json is not a human readable file, there is no need to pretty-print it.
Since this file created automatically and required by guidelines to include into git history it is severely corrupting git commitment statistics due to a terrific count of lines added by it.

@bl00mber bl00mber requested a review from a team as a code owner April 5, 2019 23:13
@bl00mber bl00mber changed the base branch from latest to release-next April 5, 2019 23:18
lib/shrinkwrap.js Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor

isaacs commented Apr 12, 2019

I have mixed feelings about this.

While it's fair that it's not primarily intended to be consumed by humans, I do typically review the changes in it, just to make sure I'm tracking which packages are added to my tree. In cases where a dep (or meta-dep) causes a test to break, I've even gotten some value in git-bisecting over its history to find the problem.

I'd also challenge the "severely corrupting" angle. Modifying the dependency tree on a project is kind of a big deal. Tracking lines in package-lock.json is sort of a middle ground between tracking the entirety of node_modules and the "yolo" approach of times past. One way to frame this is that, since deps typically make up 97% or so of LoC in an application, updating deps is a bit of Real Work just as much (or more than) modifying lines of code in the project itself.

@bl00mber
Copy link
Contributor Author

bl00mber commented Apr 12, 2019

@isaacs should I add some option to enable pretty-print? I suppose significant percentage of projects (~80-97%) not using this file much. Of course, there is a lot of other package managers and tools corrupting git statistics, but the npm is a 1st package manager (yarn downloads is 0.08% of npm's) of the 1st used programming language in the web, and package-lock.json is a required file in each project, this is probably a billions of package-lock.json files committed every week.

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2019

(ftr, it's not required; it's just enabled by default - package-lock=false in .npmrc disables it)

lib/shrinkwrap.js Outdated Show resolved Hide resolved
@dominictarr
Copy link

there is a postshrinkwrap script that you could use to de-pretty print the npm-shrinkwrap.json file... I'm not sure if it runs on package-lock though (I'm generally confused by the different behaviour between these two, tbh)

@bl00mber
Copy link
Contributor Author

bl00mber commented May 8, 2019

@dominictarr yes, this file can be modified with the use of external scripts. The issue is about default behavior. When someone adds or updates a dependency and sends a PR it is causing a mess in consistency of development history.

@isaacs isaacs force-pushed the release-next branch 2 times, most recently from 896149d to 31718e7 Compare June 29, 2019 21:55
@rhendric
Copy link
Contributor

rhendric commented Jul 8, 2019

Please, no. A pretty-printed package-lock.json is much more likely to be successfully automatically merged or rebased by git than one that's all in one line. Having the option to switch back to the multiline format after the fact isn't much help the first time you try to merge a PR containing a change to unrelated dependencies. If this became the default, both project owners and submitters of PRs to other projects would find any change to project dependencies a lot more obnoxious.

(Of course there are cases even with pretty-printing when the only way to merge package-lock.json is to blow it away and regenerate it locally. But these cases aren't all cases, as they would be if package-lock.json defaulted to a single line format.)

@isaacs
Copy link
Contributor

isaacs commented Jul 8, 2019

Yeah, I'm inclined to say no on this, having thought about it a bit more. I get the reasoning behind it, and I'd be open to an option to disable pretty-printing, but the current default is the best default for most cases.

npm-merge-driver is a nice tool to handle merge conflicts in the vast majority of cases, but would be much less effective if the JSON was all on one line.

@isaacs isaacs closed this Jul 11, 2019
@bl00mber
Copy link
Contributor Author

@isaacs so if I am allowed to suggest this non-default option I should open another PR?

@isaacs
Copy link
Contributor

isaacs commented Jul 12, 2019

@bl00mber Sure, that's fine.

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 this pull request may close these issues.

6 participants