Keep final new line in package.json

[aidant]

Platforms affected

Motivation and Context

Cordova removes the final new line in a package.json when running cordova commands. This causes us a problem since we have a formatting rule for final new lines.

Description

Apply the same writing rules every time the package.json is written to.

Testing

We have had the following patch in production for the last 3 months.

Patch

diff --git a/node_modules/cordova-lib/src/cordova/platform/addHelper.js b/node_modules/cordova-lib/src/cordova/platform/addHelper.js
index 0856245..653f834 100644
--- a/node_modules/cordova-lib/src/cordova/platform/addHelper.js
+++ b/node_modules/cordova-lib/src/cordova/platform/addHelper.js
@@ -27,6 +27,9 @@ const cordova_util = require('../util');
 const promiseutil = require('../../util/promise-util');
 const platforms = require('../../platforms');
 const detectIndent = require('detect-indent');
+const detectNewline = require('detect-newline');
+const stringifyPackage = require('stringify-package');
+const writeFileAtomicSync = require('write-file-atomic').sync;
 const getPlatformDetailsFromDir = require('./getPlatformDetailsFromDir');
 const preparePlatforms = require('../prepare/platforms');
 
@@ -228,7 +231,8 @@ function addHelper (cmd, hooksRunner, projectRoot, targets, opts) {
                 if (modifiedPkgJson === true) {
                     const file = fs.readFileSync(pkgJsonPath, 'utf8');
                     const indent = detectIndent(file).indent || '  ';
-                    fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, indent), 'utf8');
+                    const newline = detectNewline(file);
+                    writeFileAtomicSync(pkgJsonPath, stringifyPackage(pkgJson, indent, newline), { encoding: 'utf8' });
                 }
             });
         }).then(function () {
diff --git a/node_modules/cordova-lib/src/cordova/platform/remove.js b/node_modules/cordova-lib/src/cordova/platform/remove.js
index 5a91906..e8ade46 100644
--- a/node_modules/cordova-lib/src/cordova/platform/remove.js
+++ b/node_modules/cordova-lib/src/cordova/platform/remove.js
@@ -25,6 +25,9 @@ const cordova_util = require('../util');
 const promiseutil = require('../../util/promise-util');
 const platforms = require('../../platforms/platforms');
 const detectIndent = require('detect-indent');
+const detectNewline = require('detect-newline');
+const stringifyPackage = require('stringify-package');
+const writeFileAtomicSync = require('write-file-atomic').sync;
 
 module.exports = remove;
 
@@ -71,7 +74,8 @@ function remove (hooksRunner, projectRoot, targets, opts) {
                 if (modifiedPkgJson === true) {
                     const file = fs.readFileSync(pkgJsonPath, 'utf8');
                     const indent = detectIndent(file).indent || '  ';
-                    fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, indent), 'utf8');
+                    const newline = detectNewline(file);
+                    writeFileAtomicSync(pkgJsonPath, stringifyPackage(pkgJson, indent, newline), { encoding: 'utf8' });
                 }
             }
         }).then(function () {
diff --git a/node_modules/cordova-lib/src/cordova/plugin/add.js b/node_modules/cordova-lib/src/cordova/plugin/add.js
index 087bc29..95b1c10 100644
--- a/node_modules/cordova-lib/src/cordova/plugin/add.js
+++ b/node_modules/cordova-lib/src/cordova/plugin/add.js
@@ -32,6 +32,9 @@ const fs = require('fs-extra');
 const semver = require('semver');
 const url = require('url');
 const detectIndent = require('detect-indent');
+const detectNewline = require('detect-newline');
+const stringifyPackage = require('stringify-package');
+const writeFileAtomicSync = require('write-file-atomic').sync;
 const preparePlatforms = require('../prepare/platforms');
 
 module.exports = add;
@@ -154,7 +157,8 @@ function add (projectRoot, hooksRunner, opts) {
                             // Write to package.json
                             const file = fs.readFileSync(pkgJsonPath, 'utf8');
                             const indent = detectIndent(file).indent || '  ';
-                            fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, indent), 'utf8');
+                            const newline = detectNewline(file);
+                            writeFileAtomicSync(pkgJsonPath, stringifyPackage(pkgJson, indent, newline), { encoding: 'utf8' });
                         }
 
                         const src = module.exports.parseSource(target, opts);
diff --git a/node_modules/cordova-lib/src/cordova/plugin/remove.js b/node_modules/cordova-lib/src/cordova/plugin/remove.js
index f6cdf89..a9cbfe6 100644
--- a/node_modules/cordova-lib/src/cordova/plugin/remove.js
+++ b/node_modules/cordova-lib/src/cordova/plugin/remove.js
@@ -28,6 +28,9 @@ const path = require('path');
 const fs = require('fs-extra');
 const PluginInfoProvider = require('cordova-common').PluginInfoProvider;
 const detectIndent = require('detect-indent');
+const detectNewline = require('detect-newline');
+const stringifyPackage = require('stringify-package');
+const writeFileAtomicSync = require('write-file-atomic').sync;
 const { Q_chainmap } = require('../../util/promise-util');
 const preparePlatforms = require('../prepare/platforms');
 
@@ -142,7 +145,8 @@ function remove (projectRoot, targets, hooksRunner, opts) {
             // Write out new package.json with plugin removed correctly.
             const file = fs.readFileSync(pkgJsonPath, 'utf8');
             const indent = detectIndent(file).indent || '  ';
-            fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, indent), 'utf8');
+            const newline = detectNewline(file);
+            writeFileAtomicSync(pkgJsonPath, stringifyPackage(pkgJson, indent, newline), { encoding: 'utf8' });
         }
     }
 }

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[dpogue]

Thanks for the patch!

I know we'd adopted this pattern in a few places that were causing this issue before, but I guess we didn't get all of them

[dpogue]

You might need to update some test mocks or stub methods to use writeFileAtomicSync

[aidant]

I've replaced them with fs.writeFileSync because I don't think the internal structure of writeFileAtomicSync is compatible with the test structure.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.97%. Comparing base (db5c8c0) to head (4c21d44).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
+ Coverage   87.91%   87.97%   +0.06%     
==========================================
  Files          46       46              
  Lines        2134     2146      +12     
==========================================
+ Hits         1876     1888      +12     
  Misses        258      258              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aidant]

How would you like me to proceed? I've not changed the semantics around the interactions with the file system. Yet the security bot is calling out a potential race condition that already exists.

[dpogue]

yeah, I think this is probably fine to merge as is.

npm has refactored and deprecated a bunch of their tooling internally and now has a @npmcli/package-json module that handles a bunch of package.json-related stuff. In our next major we should look at refactoring our package.json handling to use that, and centralize everywhere that we read/write package.json so that we don't have to make fixes like these ones in a bunch of different spots.

[aidant]

Thanks for the quick responses, I did not expect to get these reviewed or merged today! I assume these will be included in 12.0.3 or 12.1.0?