[CB-14140] Replace shelljs calls with fs-extra & which #21

dpogue · 2018-05-15T06:18:40Z

Replaces all uses of shelljs with fs-extra for handling directory creation/deletion and file copying. Replaces the single use of shelljs.which with which.

While this doesn't have a huge impact here, fs-extra should give us some more stability in our tests in other projects like cordova-lib, especially around directory deletions on Windows.

The potential concern is that these changes will cause test failures in other modules because they are stubbing methods on fs instead of fs-extra, but my hope would be to update all those other modules as well. I've got a branch of cordova-lib that I've started updating.

codecov-io · 2018-05-15T06:23:17Z

Codecov Report

Merging #21 into master will increase coverage by 0.11%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   85.51%   85.63%   +0.11%     
==========================================
  Files          19       19              
  Lines        1761     1747      -14     
  Branches      371      367       -4     
==========================================
- Hits         1506     1496      -10     
+ Misses        255      251       -4

Impacted Files	Coverage Δ
src/PluginInfo/PluginInfo.js	`81.25% <100%> (ø)`	⬆️
src/PlatformJson.js	`78.18% <100%> (+2.74%)`	⬆️
src/PluginManager.js	`92.45% <100%> (ø)`	⬆️
src/ConfigChanges/ConfigFile.js	`89.85% <100%> (ø)`	⬆️
src/PluginInfo/PluginInfoProvider.js	`85.71% <100%> (ø)`	⬆️
src/CordovaCheck.js	`87.5% <100%> (ø)`	⬆️
src/ConfigParser/ConfigParser.js	`76.73% <100%> (ø)`	⬆️
src/util/xml-helpers.js	`94.94% <100%> (ø)`	⬆️
src/FileUpdater.js	`86.01% <100%> (-0.92%)`	⬇️
src/superspawn.js	`69.41% <66.66%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff3630f...6534c21. Read the comment docs.

raphinesse · 2018-05-21T16:45:25Z

👍 I thought about proposing the same substitution when working on cordova-create. I definitely prefer this over the shelljs solution.

dpogue · 2018-05-21T16:49:17Z

I have all tests passing here, but we still use shelljs in one spot (superspawn for shell.which).

The potential concern is that these changes will cause test failures in other modules because they are stubbing methods on fs instead of fs-extra, but my hope would be to update all those other modules as well. I've got a branch of cordova-lib that I've started updating.

raphinesse · 2018-05-21T16:55:08Z

@dpogue Regarding stubbing/mocking: foo.__set__('fs', mockFs); will replace the variable named fs in module foo's top level scope. So it should work regardless of what fs refers to.
Edit: I see, you mean when other modules do stuff like spyOn(require('fs'), ...)

Use this to get rid of shelljs? https://www.npmjs.com/package/which

dpogue · 2018-05-22T03:55:36Z

This should be ready for review now

raphinesse

Looks like a correct transformation of the existing code to me 👍

I left some suggestions on how to further improve some of the locations you touched. Some might be considered out of the scope of this PR.

raphinesse · 2018-05-22T14:59:02Z

spec/ConfigChanges/ConfigChanges.spec.js

@@ -196,10 +195,10 @@ describe('config-changes module', function () {

    describe('processing of plugins (via process method)', function () {
        beforeEach(function () {
-            shell.cp('-rf', dummyplugin, plugins_dir);
+            fs.copySync(dummyplugin, path.join(plugins_dir, path.basename(dummyplugin)));


Since this idiom occurs quite often in this file, I'd add a helper like this

function installPlugin (pluginPath) { fs.copySync(pluginPath, path.join(plugins_dir, path.basename(pluginPath))); }

raphinesse · 2018-05-22T15:14:55Z

spec/CordovaCheck.spec.js

@@ -31,12 +31,12 @@ describe('findProjectRoot method', function () {
        process.chdir(cwd);
    });
    function removeDir (someDirectory) {


I'd inline all occurrences of this helper and drop it, since it's now only an alias

Edit: I realize now that the placement of the comment was suboptimal

raphinesse · 2018-05-22T15:16:35Z

spec/CordovaCheck.spec.js

    }
    it('Test 001 : should return false if it hits the home directory', function () {
        var somedir = path.join(home, 'somedir');
        removeDir(somedir);
-        shell.mkdir(somedir);
+        fs.ensureDirSync(somedir);


In this particular instance, one could use fs.emptyDirSync(somedir) to replace the remove/create idiom

raphinesse · 2018-05-22T15:19:47Z

spec/CordovaCheck.spec.js

-        shell.mkdir('-p', path.join(anotherdir, 'www', 'config.xml'));
-        shell.mkdir('-p', path.join(somedir, 'www'));
-        shell.mkdir('-p', path.join(somedir, 'config.xml'));
+        fs.ensureDirSync(anotherdir);


We don't need this because of next line

Again, bad placement of the comment on my part 🙄

raphinesse · 2018-05-22T15:22:46Z

spec/CordovaCheck.spec.js

-        shell.mkdir('-p', anotherdir);
-        shell.mkdir('-p', path.join(somedir, 'www', 'config.xml'));
+        fs.ensureDirSync(anotherdir);
+        fs.ensureDirSync(path.join(somedir, 'www', 'config.xml'));


I think the previous version was just a lazy hack. We actually want fs.ensureFileSync on all instances of this. One might might even consider adding a test that asserts that directories are not accepted as config.xml.

raphinesse · 2018-05-22T15:31:43Z

src/FileUpdater.js

-            } else if (!targetStats.isDirectory() && sourceStats.isDirectory()) {
-                log('delete ' + targetPath + ' (source is a directory)');
-                shell.rm('-f', targetFullPath);
+            if ((targetStats.isDirectory() && !sourceStats.isDirectory()) || (!targetStats.isDirectory() && sourceStats.isDirectory())) {


Condition is equivalent to targetStats.isDirectory() !== sourceStats.isDirectory().
I'd merge with containing block to targetStats && (targetStats.isDirectory() !== sourceStats.isDirectory())

raphinesse · 2018-05-22T15:39:52Z

src/PlatformJson.js

@@ -37,7 +36,7 @@ PlatformJson.load = function (plugins_dir, platform) {
 };

 PlatformJson.prototype.save = function () {
-    shelljs.mkdir('-p', path.dirname(this.filePath));
+    fs.ensureDirSync(path.dirname(this.filePath));
    fs.writeFileSync(this.filePath, JSON.stringify(this.root, null, 2), 'utf-8');


This can be simplified to:

fs.outputJsonSync(this.filePath, this.root, {spaces: 2})

Encoding defaults to utf-8

raphinesse · 2018-05-22T15:40:24Z

src/PlatformJson.js

@@ -212,7 +211,7 @@ PlatformJson.prototype.generateMetadata = function () {
 */
 PlatformJson.prototype.generateAndSaveMetadata = function (destination) {
    var meta = this.generateMetadata();
-    shelljs.mkdir('-p', path.dirname(destination));
+    fs.ensureDirSync(path.dirname(destination));
    fs.writeFileSync(destination, meta, 'utf-8');


Again, these lines can be replaced by:

fs.outputFileSync(destination, this.generateMetadata());

dpogue · 2018-05-23T05:41:42Z

Thanks for those review comments @raphinesse! Since they all help to make the code easier to read I'll consider them all in scope 😉

I've added another commit addressing them. We'll squash this branch when merging.

raphinesse · 2018-05-23T07:21:15Z

Did you include the async test handling changes by accident? Since they are completely unrelated, I would consider them out of scope 😉

I'm meaning to say the async test handling changes are fine but should go in a separate PR IMHO.

dpogue · 2018-05-24T04:05:15Z

Fixed the re-spacing JSON output, and pulled out the spec changes for a future PR. Thanks!

raphinesse · 2018-05-24T10:48:10Z

src/PlatformJson.js

@@ -211,8 +210,7 @@ PlatformJson.prototype.generateMetadata = function () {
 */
 PlatformJson.prototype.generateAndSaveMetadata = function (destination) {
    var meta = this.generateMetadata();
-    fs.ensureDirSync(path.dirname(destination));
-    fs.writeFileSync(destination, meta, 'utf-8');
+    fs.outputJsonSync(destination, meta);


I hate to bug you again with this, but it was not about re-spacing JSON output. generateMetadata returns a string containing JS, not JSON. Therefore we need fs.outputFileSync(destination, meta) here (mind the File).

I also wrote a butt-ugly test that will fail with your implementation:

describe('generateAndSaveMetadata method', function () { it('should save generated metadata', function () { // Needs to use graceful-fs, since that is used by fs-extra const spy = spyOn(require('graceful-fs'), 'writeFileSync'); const dest = require('path').join(__dirname, 'test-destination'); platformJson.addPluginMetadata(fakePlugin).generateAndSaveMetadata(dest); expect(spy).toHaveBeenCalledTimes(1); const [file, data] = spy.calls.argsFor(0); expect(file).toBe(dest); expect(data.indexOf(JSON.stringify(platformJson.root.modules, null, 2))).toBeGreaterThan(0); expect(data).toMatch(JSON.stringify(platformJson.root.plugin_metadata, null, 2)); }); });

Ahh, now I understand! 🤦‍♂️

I've fixed that and added your test to the PlatformJson.spec.js

brodybits · 2018-05-30T21:46:00Z

Any timeline when this one will be merged?

dpogue · 2018-05-30T22:48:38Z

I'm hoping to do a minor version release of cordova-common with bugfixes before merging this, since this might qualify as a major-version API change.

dpogue · 2018-06-05T05:35:18Z

We've done the cordova-common 2.2.3 release, I think we're ready to merge this.
@raphinesse care to do the honours? 🙂

raphinesse · 2018-06-05T06:09:23Z

With the greatest of pleasure 😁

brodybits · 2018-06-15T17:45:39Z

Linking this wonderful change which landed in d781ffd to CB-14140 (https://issues.apache.org/jira/browse/CB-14140).

dpogue force-pushed the fs-extra branch 2 times, most recently from 061db2d to d84c28a Compare May 19, 2018 04:16

Replace shelljs with fs-extra

083c139

dpogue force-pushed the fs-extra branch from d84c28a to 083c139 Compare May 22, 2018 03:46

dpogue changed the title ~~WIP: Replace most shelljs calls with fs-extra~~ Replace shelljs calls with fs-extra & which May 22, 2018

raphinesse requested changes May 22, 2018

View reviewed changes

Squash: Updates from code review

d74d95c

dpogue force-pushed the fs-extra branch from 7ac3f33 to d74d95c Compare May 24, 2018 04:04

raphinesse reviewed May 24, 2018

View reviewed changes

Squash: Fix generateAndSaveMetadata output

6534c21

raphinesse approved these changes May 25, 2018

View reviewed changes

This was referenced May 29, 2018

[CB-12397] .gitignore instead of unwanted .npmignore after cordova create (7.0.1) apache/cordova-discuss#69

Closed

CB-14140 WIP fs-extra instead of shelljs apache/cordova-create#14

Closed

raphinesse merged commit d781ffd into apache:master Jun 5, 2018

brodybits changed the title ~~Replace shelljs calls with fs-extra & which~~ [CB-14140] Replace shelljs calls with fs-extra & which Jun 15, 2018

This was referenced Jul 12, 2019

feat: Build app bundles (.aab files) apache/cordova-android#764

Merged

[MAJOR] Migrate from shelljs to fs-extra apache/cordova-android#781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CB-14140] Replace shelljs calls with fs-extra & which #21

[CB-14140] Replace shelljs calls with fs-extra & which #21

dpogue commented May 15, 2018 •

edited

Loading

codecov-io commented May 15, 2018 •

edited

Loading

raphinesse commented May 21, 2018

dpogue commented May 21, 2018

raphinesse commented May 21, 2018 •

edited

Loading

dpogue commented May 22, 2018

raphinesse left a comment

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018

raphinesse May 22, 2018

raphinesse May 22, 2018

raphinesse May 22, 2018 •

edited

Loading

dpogue commented May 23, 2018

raphinesse commented May 23, 2018 •

edited

Loading

dpogue commented May 24, 2018

raphinesse May 24, 2018 •

edited

Loading

dpogue May 24, 2018

brodybits commented May 30, 2018

dpogue commented May 30, 2018

dpogue commented Jun 5, 2018

raphinesse commented Jun 5, 2018

brodybits commented Jun 15, 2018

[CB-14140] Replace shelljs calls with fs-extra & which #21

[CB-14140] Replace shelljs calls with fs-extra & which #21

Conversation

dpogue commented May 15, 2018 • edited Loading

codecov-io commented May 15, 2018 • edited Loading

Codecov Report

raphinesse commented May 21, 2018

dpogue commented May 21, 2018

raphinesse commented May 21, 2018 • edited Loading

dpogue commented May 22, 2018

raphinesse left a comment

Choose a reason for hiding this comment

raphinesse May 22, 2018 • edited Loading

Choose a reason for hiding this comment

raphinesse May 22, 2018 • edited Loading

Choose a reason for hiding this comment

raphinesse May 22, 2018

Choose a reason for hiding this comment

raphinesse May 22, 2018 • edited Loading

Choose a reason for hiding this comment

raphinesse May 22, 2018

Choose a reason for hiding this comment

raphinesse May 22, 2018

Choose a reason for hiding this comment

raphinesse May 22, 2018

Choose a reason for hiding this comment

raphinesse May 22, 2018 • edited Loading

Choose a reason for hiding this comment

dpogue commented May 23, 2018

raphinesse commented May 23, 2018 • edited Loading

dpogue commented May 24, 2018

raphinesse May 24, 2018 • edited Loading

Choose a reason for hiding this comment

dpogue May 24, 2018

Choose a reason for hiding this comment

brodybits commented May 30, 2018

dpogue commented May 30, 2018

dpogue commented Jun 5, 2018

raphinesse commented Jun 5, 2018

brodybits commented Jun 15, 2018

dpogue commented May 15, 2018 •

edited

Loading

codecov-io commented May 15, 2018 •

edited

Loading

raphinesse commented May 21, 2018 •

edited

Loading

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018 •

edited

Loading

raphinesse May 22, 2018 •

edited

Loading

raphinesse commented May 23, 2018 •

edited

Loading

raphinesse May 24, 2018 •

edited

Loading