grunt checkout-shas should npm prune and npm update

[samreid]

The instructions to prepare codebase for RC or to restore codebase are currently invasive, it would be better if grunt checkout-shas automatically did npm prune and npm update in chipper and npm prune and npm update.

grunt checkout-master should do the same.

Bonus points if we can do this for the simulation directory as well (in addition to chipper).

[samreid]

This would have to be done in a way that doesn't disrupt perennial.

[samreid]

@pixelzoom any thoughts on this before I investigate?

[pixelzoom]

Agreed that this would be nice to have in checkout-shas task, there are currently too many manual steps in the instructions.

As for what specifically checkout-shas should be doing... I'm not familiar with the subtle differences between npm prune, update, etc. But @jonathanolson raised this concern in phetsims/phet-info#39 (comment):

This is potentially a sign of a greater problem. I've been under the assumption that you can change the package.json dependencies arbitrarily and still have 'npm update' successfully install the needed dependencies.

[jonathanolson]

Also as noted in the other issue:

Would perennial's grunt checkout-shas --repo={{REPO}} be recommended (since it doesn't depend on what node_modules are in chipper)?

[samreid]

I have no plans to work on this in the near future. I'll put it up for grabs at the next developer meeting (or we can shelve it for now).

[zepumph]

I can look into this.

[zepumph]

I did a first pass at this, and it seems to work pretty well, would someone mind giving it a small review, perhaps @jonathanolson as I haven't bothered him nearly as much as I have @pixelzoom and @samreid recently?

One thing I found while doing this was that grunt checkout-master probably won't npm prune and update because this new code isn't in the old chipper sha, so found (and will document) that it is best to run `grunt checkout-master twice, because the second time will be from chipper's master and will then npm prune and update. Unless there is a better thought.

[samreid]

I ran checkout shas today and was happy to see the npm prune and update automatically happening, thanks! I'll assign to @jonathanolson to see if he has time for a closer review.

[jonathanolson]

From reading the code, it looks like chipper would be pruned/updated 10 times if there are 10 repos included in dependencies.json?

Can this be changed so chipper is only pruned/updated 1 time? (Also, presumably chipper is included in all dependencies.jsons?)

[zepumph]

From reading the code, it looks like chipper would be pruned/updated 10 times if there are 10 repos included in dependencies.json?

I'm unsure what you mean, the prune and update command is called only when all repos have been checked out:

          if ( numToCheckOut === numCheckedOut ) {
            pruneAndUpdate();
          }

Also I have noticed while running it that npm prune/update are only happening at the end, once for chipper and once for the simRepo.

@jonathanolson could you be more specific about the problem you are seeing?

[jonathanolson]

Sorry, I was incorrectly thinking multiple repos would need npm update. That looks good to me.

Some general notes:

numCheckedOut++;

is generally easier to read than

numCheckedOut = numCheckedOut + 1;

Iteration over 'properties' is done multiple times by filtering out comments and self. At the top:

var dependenciesToCheckOut = Object.keys( dependencies ).filter( function( repository ) {
  return repository !== repositoryName && repository !== 'comment';
} );

The hasOwnProperty check is not needed, since it's from loaded JSON (no prototype funkiness).

Then, a name like 'dependencyName' would be much more readable than 'property'.

assert( typeof( dependencies[ property ].branch !== 'undefined' ) && typeof( dependencies[ property ].sha !== 'undefined' ) );

could be somewhat simplified to

assert( dependencies[ dependencyName ].branch && dependencies[ dependencyName ].sha );

since valid branch/head names and SHAs should be truthy.

I also see error1, stdout1 and stderr1, but no error2, etc. Can these be renamed to error, stdout and stderr?

[zepumph]

@jonathanolson thanks for the recommendations, closing.

[zepumph]

I noticed that perennial's grunt checkout-shas could potentially use this upgrade as well. Do we want update perennial to npm prune and update as well? @jbphet @mattpen what do you guys think?

[mattpen]

npm prune and npm update are called explicitly in build-server. Do we need to improve this process?

See: https://github.com/phetsims/perennial/blob/1867a7f688698f74cfce9e2363ecaefe4c1c1c2d/js/build-server/build-server.js#L866

[zepumph]

I think that's fine in fact I think it's better.This means that there is a disconnect between chipper and perennial here in the grunt rule ( one conducts npm prune/update and the other does not), but I think that this is much more explicit and easier to maintain for the build-server application. Closing.

[samreid]

If our grunt builds always had this feature, then it would have simplified perennial. Since legacy maintenance builds don't have auto-prune and update, perennial needs this functionality. Once 100% of our published sims are using new grunt, we could simplify perennial, but this may not happen for a long long time.