-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: use detect-newline to detect newline instead of default \n #19904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time and initiative to do this!! There's a bit more that needs to go into this patch for it to be ready, and I'm honestly fine not having test for a thing like this, provided the regular test suite continues passing.
lib/install/save.js
Outdated
@@ -122,7 +124,7 @@ function savePackageJson (tree, next) { | |||
tree.package.bundleDependencies = deepSortObject(bundle) | |||
} | |||
|
|||
var json = JSON.stringify(tree.package, null, indent) + '\n' | |||
var json = JSON.stringify(tree.package, null, indent) + newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not replace all the newlines in the JSON file. I'm pretty sure you're gonna have to JSON.stringify(...).replace(/\n/g, newline)
here, not just append the final one.
@@ -3,6 +3,7 @@ | |||
const createShrinkwrap = require('../shrinkwrap.js').createShrinkwrap | |||
const deepSortObject = require('../utils/deep-sort-object.js') | |||
const detectIndent = require('detect-indent') | |||
const detectNewline = require('detect-newline') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the only file that needs you to do this. You'll also have to add this to lib/shrinkwrap.js
and lib/version.js
, which both write out json files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i updated the PR to cover lib/shrinkwrap.js
and lib/version.js
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a bunch for this patch!
#18943 from 4 months ago solved this in IMO a cleaner way and included tests. Guess you didn't see it... |
use
detect-newline
to detect newline instead of using the default\n
which may cause npm to find thatpackagejson
string to be different even though there is no change. (causing by the\n
newline)