This is looking good, Thiago. Thank you! As we discussed, I'm OK with holding off on tests. The last non-trivial thing I'd like to see is removing the code that writes properties. I give two possible approaches below.
I'm happy to have a call if it helps, or leave it to you, as you prefer. Thanks! Gary https://codereview.appspot.com/6821090/diff/15005/Makefile File Makefile (right): https://codereview.appspot.com/6821090/diff/15005/Makefile#newcode82 Makefile:82: @./bin/write-properties true I really would like to see this (and the corresponding line in 87 and the corresponding "write-properties" script) go. My preferred approach, which we talked about yesterday, would be to make the server always have both the debug version (debug.html?) and the production version (index.html) available, by having the server dynamically change the js files that index.html points to. Alternatively, server.js would accept an argument to turn on debug mode. By default, it would run in prodcution mode. https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js File app/modules-debug.js (right): https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js#newcode1 app/modules-debug.js:1: GlobalConfig = { An idea (as in, do this if you agree): it would be nice to have a comment at the top of this file saying that it is only used for the debug (developer) views of the application, and pointing to the mechanism that is used (e.g., in server.js) to serve this file rather than the other one. Maybe it's overkill. I'd do it, but I'd be fine with accepting disagreement. https://codereview.appspot.com/6821090/diff/15005/app/modules.js File app/modules.js (right): https://codereview.appspot.com/6821090/diff/15005/app/modules.js#newcode4 app/modules.js:4: ignoreRegistered: true, Is there a setting we can use to say "never download anything from the internet, ever"? If so, I suggest we use that both for this and the debug modules file. Is it bootstrap: false? Or is this already handled somehow? https://codereview.appspot.com/6821090/diff/15005/bin/merge-files File bin/merge-files (right): https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode1 bin/merge-files:1: #!/usr/bin/env node These changes look great to me. Thanks. https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode4 bin/merge-files:4: * We should aggregate and minimize the js sources in order to improve the load suggestion: delete "should". (We are doing it. :-) ) https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode8 bin/merge-files:8: * to run from only static files and we want to be able to run behaind a "behind" (I realize these are probably my fault. sorry :-) ) https://codereview.appspot.com/6821090/diff/15005/bin/write-properties File bin/write-properties (right): https://codereview.appspot.com/6821090/diff/15005/bin/write-properties#newcode1 bin/write-properties:1: #!/usr/bin/env node As I said, I'd like to delete. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js File lib/merge-files.js (right): https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode14 lib/merge-files.js:14: // end. Another option (up to you): remove this comment, and do the export of each function as you define it ("expose.minify = function(file) {...}" for instance) or immediately after you define it. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode29 lib/merge-files.js:29: // or not) Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode44 lib/merge-files.js:44: // ignores Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode75 lib/merge-files.js:75: // it is Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode86 lib/merge-files.js:86: // (as Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode128 lib/merge-files.js:128: // these Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/ -- https://code.launchpad.net/~tveronezi/juju-gui/uglify/+merge/133116 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/uglify into lp:juju-gui. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
