search

ucla-oarc-mobile / mwf

UCLA Mobile Web Framework
http://mwf.ucla.edu
Other
86 stars 25 forks source link

Refactor vars.php for testing and maintainability #107

Closed Trott closed 12 years ago

Trott commented 12 years ago

Encapsulation, separation of PHP from JS, unit tests for the PHP pieces.

ebollens commented 12 years ago

Keep in mind that DTs, whenever we get it into production, will replace the current implementation in some way.

Trott commented 12 years ago

Eric, can you comment on the following for review/feature/js_vars_helper?

  1. Is vars.php (which I have left unminified for now) easier to minify in this format? Can you tell me your procedure so I don't have to reinvent the wheel and I'll see about minifying it.
  2. While DTS will reduce the amount of code and logic in what is now vars.php/JS_Vars_helper, it won't eliminate it entirely, will it? If it will, hey, awesome.
  3. Anything else you care to comment on.
ebollens commented 12 years ago
  1. Minification is done manually in vars.php by shrinking the file contents.
  2. It definitely won't eliminate it. It will simply reduce it slightly. The standalone DTS (https://github.com/ebollens/dts) offloads all variable output into a separate JS include (see federation.php). It's a very minimal subset of what we're looking at with the current MWF implementation. Of course, MWF also provides a lot of other variables, but there are some that will be considered redundant come the merge of DTS into MWF.
Trott commented 12 years ago

Manual minification? Dude, I feel like I don't even know you.

Just kidding. Sort of.

Well, as if JS_Vars_Helper needed any help selling itself, here's an automated minifier for vars.php called extract_php_tokens.php. Whoops, I should rename that. Still, yay, no more manual minification for vars.php.

7af4770b69e4fbf23573a6f4cf481d8db61f6195

I'd really, really, really like to merge this into develop (both the minifier and the JS_Vars_Helper object). How are you feeling about it?

ebollens commented 12 years ago

Let me review it over the weekend, and then we should be good to next week.

Trott commented 12 years ago

2221eac19556ad5b86b3f62d1eaa20f835ff41e0 has a minor update to a comment.

Trott commented 12 years ago

And an elimination of commented code in eb202bdf93e625250de6f76ce3cf4208637b3bf0. Hooray for minutiae,

ebollens commented 12 years ago

Zorayr and I spoke about minifiers a couple weeks ago when we were trying to do something JIT for DTS, and what we arrived at was that we didn't like the idea of adding another dependency to the stack like Java, even if it's just a dev tool.

Therefore, I would say I like where we're headed with JS_Vars_Helper, but I don't like the addition of YUI. Can you use a PHP minifier instead?

Trott commented 12 years ago

Added unit tests, restored @author tag per your comment.

Trott commented 12 years ago

The main issue with using a PHP-based minifier is that they simply do not minify as aggressively as YUI. I'll make it a command-line option that defaults to JSMin but will let you specify YUI if you want it.

By the way, if you're looking to remove things that aren't PHP/JS/CSS/HTML from the app, don't forget about install.sh.

Trott commented 12 years ago

OK, I added a -m flag to select yui or jsmin on the command line. b5e2f5f27aa9839a922fd70f784ee25bb38dbb2f I had to update the jsmin.class.php implementation to one that preserves comments that are marked important (i.e., start with /! rather than just /).