Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add postinstall script for apps under cfgov-refresh #3831

Merged
merged 1 commit into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion gulp/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ module.exports = {
build: [
'config/**/*.js',
'gulpfile.js',
'gulp/**/*.js'
'gulp/**/*.js',
'scripts/npm/**/*.js'
]
},
test: {
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@
"wcag": "0.3.0"
},
"scripts": {
"test": "snyk test"
"test": "snyk test",
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this, PR, but this should probably be changed to run our actual test suites.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we have #3576 and #2303 that overlap with that.

"postinstall": "node scripts/npm/apps-install"
},
"snyk": true
}
39 changes: 39 additions & 0 deletions scripts/npm/apps-install/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
This script handles installing node dependencies for a project that lives
under cfgov-refresh, but has its own package.json. These projects appear
under the ./cfgov/unprocessed/apps/ path.
*/

const fs = require( 'fs' );
const exec = require( 'child_process' ).exec;

const paths = require( '../../../config/environment' ).paths;

// Aggregate application namespaces that appear in unprocessed/apps.
// eslint-disable-next-line no-sync
let apps = fs.readdirSync( `${ paths.unprocessed }/apps/` );

// Filter out .DS_STORE directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial: This could be more broadly stated "Filter out hidden directories"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #3834

apps = apps.filter( dir => dir.charAt( 0 ) !== '.' );

// Run each application's JS through webpack and store the gulp streams.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually happening? Looks like all it's doing is running npm install

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #3834

apps.forEach( app => {
/* Check if node_modules directory exists in a particular app's folder.
If it doesn't log the command to add it and don't process the scripts. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial: Could use a comma after "doesn't" for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #3834

const appsPath = `${ paths.unprocessed }/apps/${ app }`;
// eslint-disable-next-line no-sync
if ( fs.existsSync( `${ appsPath }/package.json` ) ) {
exec( `npm --prefix ${ appsPath } install ${ appsPath }`,
( error, stdout, stderr ) => {
// eslint-disable-next-line no-console
console.log( 'App\'s npm output: ' + stdout );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use app in these logs to have it say what app it's actually referring to, here? Could get hard to figure things out if this is ever installing multiple apps' stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #3834

// eslint-disable-next-line no-console
console.log( 'App\'s npm errors: ' + stderr );
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there always errors/warnings? Should this get a condition check like error below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #3834

if ( error !== null ) {
// eslint-disable-next-line no-console
console.log( 'App\'s exec error: ' + error );
}
}
);
}
} );