Code review comment for lp:~benji/juju-gui/npm-cache

Revision history for this message
Gary Poster (gary) wrote :

LGTM with responses--changes preferred, but I'm open to counterarguments
instead--to comments.

Thank you! This is cool to have.

Gary

https://codereview.appspot.com/8686048/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/8686048/diff/1/Makefile#newcode98
Makefile:98: NPM_CACHE_VERSION=$(shell date +%s)# Seconds since the
epoch.
I would prefer to connect this to the revid, since we have shrinkwrap
now. Is there a reason to prefer the date?

https://codereview.appspot.com/8686048/diff/1/Makefile#newcode553
Makefile:553: $(MAKE) clean-all
We really have to do that even if you specify a different cache location
in an environment variable? Yuck. :-(

https://codereview.appspot.com/8686048/diff/1/Makefile#newcode591
Makefile:591: main-doc npm-cache npm-cache-file npm-cache-file-signature
prep prod \
I wish you had divided things up the way you describe here--or at least
the file and the npm-cache. Would you be willing to add those in? If
not, clean these out.

https://codereview.appspot.com/8686048/

« Back to merge proposal