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

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

Closed
wants to merge 5 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f5e29aa
helper.js: GetFolders -> GetModules, add package.json existence check
Mar 21, 2019
01980ad
Code review fixes
Mar 22, 2019
fe15234
async version of GetModules
Mar 22, 2019
5ac8857
Rework GetModules to async/await syntax
Mar 25, 2019
858e431
GetModules: log error
Mar 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions scripts/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,17 +145,21 @@ const GetDepTree = ( name ) => {


/**
* Get all folders within a given path
* Get all modules within a given path.
* Module is folder with package.json
*
* @param {string} thisPath - The path that contains the desired folders
* @param {boolean} verbose - Verbose flag either undefined or true
*
* @return {array} - An array of names of each folder
*/
const GetFolders = ( thisPath, verbose ) => {
const GetModules = ( thisPath, verbose ) => {
try {
let folders = Fs.readdirSync( thisPath ).filter(
thisFile => Fs.statSync(`${ thisPath }/${ thisFile }`).isDirectory()
thisFile => {
let path = `${ thisPath }/${ thisFile }`;
return Fs.statSync(path).isDirectory() && Fs.existsSync(`${path}/package.json`)
Copy link
Contributor

@azerella azerella Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ghost ghost Mar 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

@azerella azerella Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
).filter(
thisFile => thisFile !== 'core'
);
Expand Down Expand Up @@ -620,7 +624,7 @@ HELPER.generate = (() => {
*/
init: () => {
const packagesPath = Path.normalize(`${ __dirname }/../packages/`);
const allModules = GetFolders( packagesPath );
const allModules = GetModules( packagesPath );

HELPER.generate.json( allModules );
HELPER.generate.index( allModules );
Expand Down Expand Up @@ -889,7 +893,7 @@ HELPER.test = (() => {
return {
init: () => {
const packagesPath = Path.normalize(`${ __dirname }/../packages/`);
const allModules = GetFolders( packagesPath );
const allModules = GetModules( packagesPath );

HELPER.test.dependencies( allModules );
HELPER.test.packagejson( allModules );
Expand Down