Add postinstall script for apps under cfgov-refresh #3831
Conversation
anselmbradford
requested review from
chosak,
Scotchester,
jimmynotjim and
sebworks
|
| @@ -101,7 +101,8 @@ | |||
| "wcag": "0.3.0" | |||
| }, | |||
| "scripts": { | |||
| "test": "snyk test" | |||
| "test": "snyk test", | |||
There was a problem hiding this comment.
Unrelated to this, PR, but this should probably be changed to run our actual test suites.
|
Also not directly related to this PR: Is there a way to bubble the |
| // eslint-disable-next-line no-sync | ||
| let apps = fs.readdirSync( `${ paths.unprocessed }/apps/` ); | ||
|
|
||
| // Filter out .DS_STORE directory. |
There was a problem hiding this comment.
Trivial: This could be more broadly stated "Filter out hidden directories"
| // Filter out .DS_STORE directory. | ||
| apps = apps.filter( dir => dir.charAt( 0 ) !== '.' ); | ||
|
|
||
| // Run each application's JS through webpack and store the gulp streams. |
There was a problem hiding this comment.
Is this actually happening? Looks like all it's doing is running npm install
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "lockfileVersion": 1 | |||
| } | |||
There was a problem hiding this comment.
The creation of this file appears to be a mistake.
There was a problem hiding this comment.
No, it should be created. I'm not sure why it's empty though.
There was a problem hiding this comment.
Look at the path; it's in the wrong location.
| exec( `npm --prefix ${ appsPath } install ${ appsPath }`, | ||
| ( error, stdout, stderr ) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'App\'s npm output: ' + stdout ); |
There was a problem hiding this comment.
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.
| // eslint-disable-next-line no-console | ||
| console.log( 'App\'s npm output: ' + stdout ); | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'App\'s npm errors: ' + stderr ); |
There was a problem hiding this comment.
Are there always errors/warnings? Should this get a condition check like error below?
| // Run each application's JS through webpack and store the gulp streams. | ||
| 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. */ |
There was a problem hiding this comment.
Trivial: Could use a comma after "doesn't" for clarity.
Additions
scripts/npm/apps-install/index.jsscript for runningnpm installon./cfgov/unprocessed/apps/[app namespace]/package.json.Testing
./cfgov/unprocessed/apps/[app namespace]/node_modulesgulp build, getApp dependencies not installed…message. (Do not follow message.)npm installfrom the project root.gulp buildand check that./cfgov/unprocessed/apps/[app namespace]/node_modulesexists.