helper.js: GetFolders -> GetModules, add package.json existence check #697

ghost · 2019-03-21T20:28:37Z

Suggestion to #696

Replace of GetFolders function.

azerella

@toxamiz good stuff!, I've left some review comments, let me know what you think 👍

azerella · 2019-03-21T23:17:12Z

scripts/helper.js

-				thisFile => Fs.statSync(`${ thisPath }/${ thisFile }`).isDirectory()
+				thisFile => {
+					let path = `${ thisPath }/${ thisFile }`;
+					return Fs.statSync(path).isDirectory() && Fs.existsSync(`${path}/package.json`)


If we are going to refactor this function I think making it asynchronous would be really beneficial. Currently a lot of the code in helper.js is synchronous and as a good rule of thumb for any I/O this stuff should be async.

I like the readability changes but if we're updating this, we should make it really nice

Yes, there are a lot of stuff which can be run asynchronous.

OK, i can "promisify" this function and change calling code.

scripts/helper.js

ghost · 2019-03-22T14:14:51Z

updated to async version

azerella · 2019-03-21T23:31:05Z

scripts/helper.js

-				thisFile => Fs.statSync(`${ thisPath }/${ thisFile }`).isDirectory()
+				thisFile => {
+					let path = `${ thisPath }/${ thisFile }`;
+					return Fs.statSync(path).isDirectory() && Fs.existsSync(`${path}/package.json`)


Suggested change

return Fs.statSync(path).isDirectory() && Fs.existsSync(`${path}/package.json`)

return Fs.statSync( path ).isDirectory() && Fs.existsSync( `${path}/package.json` )

We might need to put a Path.normalize( ${path}/package.json ) here also

azerella

A few things still need to be addressed:

Keep an eye out on the spacing with arguments, function(arg1) should be function( arg1 ). To be fair we should provide contributors with a coding style guide, we'll look at this in the future.
Since we are refactoring we should also update the Fs.statSync and Fs.existsSync functions.
Rather than promisifying the functions, we'd prefer, for readability sake to use the async/await syntax. Node has an experimental API at the moment that wraps the fs module in promises. e.g

const Fsp = require('fs').promises;

( async() => {
    let file = await Fsp.readFile( `file.txt`, `utf-8` );
    let fileExists = ( await Fsp.stat( `file.txt` ) ).isDirectory();
}();

azerella · 2019-03-24T22:47:14Z

scripts/helper.js

-	}
+const GetModules = ( thisPath, verbose ) => {
+	return new Promise(resolve => {
+		Readdir(thisPath)


Suggested change

Readdir(thisPath)

Readdir( thisPath )

azerella · 2019-03-24T22:47:19Z

scripts/helper.js

-		return [];
-	}
+const GetModules = ( thisPath, verbose ) => {
+	return new Promise(resolve => {


Suggested change

return new Promise(resolve => {

return new Promise( resolve => {

azerella · 2019-03-24T22:47:37Z

scripts/helper.js

+const GetModules = ( thisPath, verbose ) => {
+	return new Promise(resolve => {
+		Readdir(thisPath)
+			.then(dirContent => {


Suggested change

.then(dirContent => {

.then( dirContent => {

azerella · 2019-03-24T22:47:44Z

scripts/helper.js

+		Readdir(thisPath)
+			.then(dirContent => {
+				let folders = dirContent
+					.filter(folder => {


Suggested change

.filter(folder => {

.filter( folder => {

azerella · 2019-03-24T22:48:28Z

scripts/helper.js

+				let folders = dirContent
+					.filter(folder => {
+						let path = Path.normalize( `${ thisPath }/${ folder }` );
+						return Fs.statSync( path ).isDirectory() && Fs.existsSync( Path.normalize( `${path}/package.json` ) )


This function could be refactored to read nicer and is also still synchronous and IO based.

azerella · 2019-03-24T22:48:38Z

scripts/helper.js

+						let path = Path.normalize( `${ thisPath }/${ folder }` );
+						return Fs.statSync( path ).isDirectory() && Fs.existsSync( Path.normalize( `${path}/package.json` ) )
+					})
+					.filter(folder => folder !== 'core');


Suggested change

.filter(folder => folder !== 'core');

.filter( folder => folder !== 'core' );

azerella · 2019-03-24T22:48:50Z

scripts/helper.js

+					})
+					.filter(folder => folder !== 'core');
+
+				resolve(['core', ...folders ]); // moving core to top


Suggested change

resolve(['core', ...folders ]); // moving core to top

resolve( [ 'core', ...folders ] ); // moving core to top

ghost · 2019-03-25T20:24:12Z

Hey, @adamzerella

Updated to async/await syntax. But there is one big question, see below.

Since we are refactoring we should also update the Fs.statSync and Fs.existsSync functions.

Async Fs.exists is deprecated. The only way to check file existence asynchronously is to use Fs.access. The main issue is Fs.access provides not obvious behavior, because it's a void function (return undefined), that throws an error if file is not exists. And we don't need to check that package/component is a directory via Fs.stat and straight check access to package/component/package.json.

See last commit, what you think?

Rather than promisifying the functions, we'd prefer, for readability sake to use the async/await syntax. Node has an experimental API at the moment that wraps the fs module in promises

It is node 10 experimental API, but your minimum requirement is node 8 (readme).

azerella

This PR still needs has some linting issues.

It would be cleaner to stick with one fs import and just access the function access() and readdir() as needed. I'm curious if the GetModules() function needs to be updated now that it is async and other functions that call it like init: () now being init: async ()

scripts/helper.js

ghost · 2019-03-27T08:13:02Z

Updated.

I'm curious if the GetModules() function needs to be updated now that it is async and other functions that call it like init: () now being init: async ()

Not necessary for now. All init functions are used synchronously

azerella · 2019-05-01T05:26:49Z

@toxamiz I'm going to close this PR as it doesn't resolve the original issue it was referencing #696. It still has some outstanding formatting issues, this isn't your fault. As you brought up in #710 we should have linting functionality to address this but that carries over other issues as we've previously discussed.

The helper.js file is going to be refactored in the future and possibly removed, we just aren't there yet.

helper.js: GetFolders -> GetModules, add package.json existence check

f5e29aa

azerella suggested changes Mar 21, 2019

View reviewed changes

azerella mentioned this pull request Mar 22, 2019

helper.js add check package.json existence before require #696

Closed

3 tasks

Anton added 2 commits March 22, 2019 14:17

Code review fixes

01980ad

async version of GetModules

fe15234

azerella suggested changes Mar 24, 2019

View reviewed changes

azerella added enhancement New feature or request. good first issue A good issue to work on first. maintenance General upgrades or updates. labels Mar 24, 2019

Rework GetModules to async/await syntax

5ac8857

azerella suggested changes Mar 26, 2019

View reviewed changes

scripts/helper.js Outdated Show resolved Hide resolved

GetModules: log error

858e431

azerella closed this May 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helper.js: GetFolders -> GetModules, add package.json existence check #697

helper.js: GetFolders -> GetModules, add package.json existence check #697

ghost commented Mar 21, 2019

azerella left a comment •

edited

Loading

azerella Mar 21, 2019

ghost Mar 22, 2019

ghost commented Mar 22, 2019

azerella Mar 21, 2019

azerella left a comment

azerella Mar 24, 2019

azerella Mar 24, 2019

azerella Mar 24, 2019

azerella Mar 24, 2019

azerella Mar 24, 2019

azerella Mar 24, 2019

azerella Mar 24, 2019

ghost commented Mar 25, 2019 •

edited by ghost

Loading

azerella left a comment

ghost commented Mar 27, 2019

azerella commented May 1, 2019

	return Fs.statSync(path).isDirectory() && Fs.existsSync(`${path}/package.json`)
	return Fs.statSync( path ).isDirectory() && Fs.existsSync( `${path}/package.json` )

	return new Promise(resolve => {
	return new Promise( resolve => {

	.filter(folder => folder !== 'core');
	.filter( folder => folder !== 'core' );

	resolve(['core', ...folders ]); // moving core to top
	resolve( [ 'core', ...folders ] ); // moving core to top

helper.js: GetFolders -> GetModules, add package.json existence check #697

helper.js: GetFolders -> GetModules, add package.json existence check #697

Conversation

ghost commented Mar 21, 2019

azerella left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 22, 2019

Choose a reason for hiding this comment

azerella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 25, 2019 • edited by ghost Loading

azerella left a comment

Choose a reason for hiding this comment

ghost commented Mar 27, 2019

azerella commented May 1, 2019

azerella left a comment •

edited

Loading

ghost commented Mar 25, 2019 •

edited by ghost

Loading