Skip to content
This repository was archived by the owner on Aug 11, 2022. It is now read-only.

Conversation

tuananh
Copy link
Contributor

@tuananh tuananh commented Feb 24, 2018

use detect-newline to detect newline instead of using the default \n which may cause npm to find that packagejson string to be different even though there is no change. (causing by the \n newline)

@tuananh tuananh requested a review from a team as a code owner February 24, 2018 02:05
Copy link
Contributor

@zkat zkat left a 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.

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

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')
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@zkat zkat left a 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!

@zkat zkat changed the base branch from latest to release-next February 24, 2018 05:07
@zkat zkat merged commit e74d843 into npm:release-next Feb 24, 2018
@nwoltman
Copy link
Contributor

#18943 from 4 months ago solved this in IMO a cleaner way and included tests. Guess you didn't see it...

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

Successfully merging this pull request may close these issues.

3 participants