[MAJOR] Migrate from shelljs to fs-extra

[brodybits]

As discussed during review of PR #764 (aab support). This idea originally comes from Apache CB-14140 (jira) which was done on the common/library packages. Here are some PRs to check out as examples:

• [CB-14140] Replace shelljs calls with fs-extra & which cordova-common#21
• CB-14140 Switch to using fs-extra in favour of shelljs cordova-lib#613
• [CB-14140] Remove shelljs cordova-fetch#23
• CB-14140 Use fs-extra instead of shelljs cordova-create#19

One thing to watch out for: We did encounter apache/cordova#121 on Windows, which was eventually solved by a workaround on fs-extra@8.

A few more pointers:

For spawning external processes, we migrated from Node.js “child_process” to cross-spawn in apache/cordova-common#50, started discussing further work in apache/cordova#16.

From @erisu (quoted from #764 (comment); see apache/cordova#79 for larger discussion on deprecating at least Node.js 6):

Additionally, the next major will deprecates deprecates node 6 and 8 which allows us to leverage modern JS to cleanly write a recursive directory scan.

Example: https://stackoverflow.com/a/45130990/1644129

[janpio]

Re the included comment: I don't think Cordova Android 9.0.0 was planned to drop Node 8.

[dpogue]

Yes, node 8 EOL is in December, so the thinking was that by dropping both 6 and 8 at the same time (targeting a release roughly somewhere in the fall) we’d avoid needing to do two major version bumps and go through all the release work again so soon.

EDIT: This hasn’t been decided yet, and should probably be brought up on the list for discussion.

[breautek]

If no one has already started working on this, this is something I'm willing to tackle over my available weekends.

[timbru31]

Go for it, every contribution is much appreciated :)

[breautek]

Yes, node 8 EOL is in December, so the thinking was that by dropping both 6 and 8 at the same time (targeting a release roughly somewhere in the fall) we’d avoid needing to do two major version bumps and go through all the release work again so soon.

EDIT: This hasn’t been decided yet, and should probably be brought up on the list for discussion.

There was a rejection to drop Node 8 at the same time as 6, in the sense that Node 8 should only be dropped in December.

https://lists.apache.org/thread.html/735c5ebc78fea19dd3f86067f7d330d5f3c0c660c6ed7c9b538540ac@%3Cdev.cordova.apache.org%3E

If the 9.0 milestone is only going to be release after December, then I can continue with this ticket with the assumption that node 8 support will be dropped.

[breautek]

// checks if file exists and then deletes. Error if doesn't exist
function removeFile (project_dir, src) {
    var file = path.resolve(project_dir, src);
    shell.rm('-Rf', file);
}

// deletes file/directory without checking
function removeFileF (file) {
    shell.rm('-Rf', file);
}

@brodybits

Do you think these two functions could be merged into one? They both appear to be doing essentially the same thing, and I believe the comments are also out-dated... as removeFile doesn't do anything special about checking for existence, and if it does throw an error, so would removeFileF.

[brodybits]

Do you think these two functions could be merged into one?

Hmm ... the difference I see is that one uses path.resolve while the other does not. From a quick look through bin/templates/cordova/lib/pluginHandlers.js I did not see a consistent pattern.

I would probably favor merging these functions, maybe with some kind of a note.

I did took a quick look though git blame bin/templates/cordova/lib/pluginHandlers.js and discovered use of removeFileF was introduced in 4002822 (and touched in 0ac822c to fix line endings). I tried git show -b 400282282, so far did not see anything different from HEAD in terms of removeFile* usage.

[breautek]

All cases of removeFileF that I saw used passed in a path.resolve output anyway. These functions are not exported so I'm thinking it is safe to refactor. I essentially removed removeFile and renamed removeFileF to removeFile.

[breautek]

cordova-android/bin/lib/create.js

Lines 75 to 95 in c35a990

	// Don't fail if there are no old jars.
	exports.setShellFatal(false, function () {
	shell.ls(path.join(app_path, 'libs', 'cordova-*.jar')).forEach(function (oldJar) {
	console.log('Deleting ' + oldJar);
	shell.rm('-f', oldJar);
	});
	var wasSymlink = true;
	try {
	// Delete the symlink if it was one.
	fs.unlinkSync(nestedCordovaLibPath);
	} catch (e) {
	wasSymlink = false;
	}
	// Delete old library project if it existed.
	if (shared) {
	shell.rm('-rf', nestedCordovaLibPath);
	} else if (!wasSymlink) {
	// Delete only the src, since Eclipse / Android Studio can't handle their project files being deleted.
	shell.rm('-rf', path.join(nestedCordovaLibPath, 'src'));
	}
	});

These snippet of contains a lot of shell usages, and where it uses ls I've replaced the implementation using readdirsSync where necessary, but in this instance I think this is legacy code that can simply be removed. The code appears to be looking for old jar files that must of existed 6+ years ago but is no longer required. The commit that adds this check & jar removal can be found here. It is referencing a JIRA id that doesn't appear to exist anymore. I think anybody today won't have these old jar files to begin with.

[brodybits]

Hi @breautek, I wonder if you should raise a draft PR with your ongoing work on this?

I think a draft PR with your ideas would be much easier for us to discuss than random pointers to the old code.

Thanks for all of your work on this so far!

[breautek]

Think that might be a good idea, but I'll at least refactor everything that is simple enough to refactor. Then if there is any edge cases, I may skip over them until I can point them out in the draft for discussion.

[breautek]

I should a draft PR for this sometime next week.

Just finished refactoring, and fixing all the errors and messes I've made in doing so, at least back to the point where the unit tests is all green. Next week I'll be manually testing my branch and if I'm happy with it, a PR will soon follow.