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.
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#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.
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 write-propertie s true
Makefile:82: @./bin/
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 debug.js (right):
File app/modules-
https:/ /codereview. appspot. com/6821090/ diff/15005/ app/modules- debug.js# newcode1 debug.js: 1: GlobalConfig = {
app/modules-
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 properties (right):
File bin/write-
https:/ /codereview. appspot. com/6821090/ diff/15005/ bin/write- properties# newcode1 properties: 1: #!/usr/bin/env node
bin/write-
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 files.js: 14: // end.
lib/merge-
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 files.js: 29: // or not)
lib/merge-
Trivial: Please adjust the comment formatting.
https:/ /codereview. appspot. com/6821090/ diff/15005/ lib/merge- files.js# newcode44 files.js: 44: // ignores
lib/merge-
Trivial: Please adjust the comment formatting.
https:/ /codereview. appspot. com/6821090/ diff/15005/ lib/merge- files.js# newcode75 files.js: 75: // it is
lib/merge-
Trivial: Please adjust the comment formatting.
https:/ /codereview. appspot. com/6821090/ diff/15005/ lib/merge- files.js# newcode86 files.js: 86: // (as
lib/merge-
Trivial: Please adjust the comment formatting.
https:/ /codereview. appspot. com/6821090/ diff/15005/ lib/merge- files.js# newcode128 files.js: 128: // these
lib/merge-
Trivial: Please adjust the comment formatting.
https:/ /codereview. appspot. com/6821090/