Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Makefiles for (in-place) minification #65

Closed
wants to merge 1 commit into from
Closed

Adding Makefiles for (in-place) minification #65

wants to merge 1 commit into from

Conversation

nbraud
Copy link

@nbraud nbraud commented Oct 30, 2013

Solves issues #60 and #53 (except for the GIF animation).

@ulikoehler
Copy link

👍
Is there any specific reason why you chose the closure compiler? I'd usually prefer uglifyjs.

It would be a nice feature if the PHP code itself would minify on-demand, e.g. by using https://github.com/smallhadroncollider/uglify-php

@nbraud
Copy link
Author

nbraud commented Oct 30, 2013

I don't have any specific preference about the minifier.
Actually, I don't know much about JS (except that I compile OCaml code to JS), and just took a JS source-to-source compiler that I was told was good.

However, I don't believe that we should minify on demand the Javascript sources, because:

  1. This hurts security (by increasing the attack surface).
  2. We would have to roll our own caching system (which will probably be subject to race-conditions) or rely on caching on the webserver's part (which requires some additional configuration to do right) to get decent performance.
  3. It feels pretty stupid to go through PHP to deliver static content.

However, knock yourself out and send a PR to nbraud/ZeroBin if you want to modify this PR in other ways ;-)

@ulikoehler
Copy link

I agree with dynamic minification not being the thing we want in the way you described it.
I think it shouldn't be minified on-demand, but once, if the minified file does not exist (one could just add a file exists check somewhere in the PHP code). The performance won't really suffer then (it's a static file after all), but I think the goal should be to avoid users having to install any kind of minifier / third-party software just to get ZeroBin running.

One of the things I like most about ZeroBin is it's easy to setup. If there will be infrequent releases (as before), users will need to use some kind of GitHub code, in which case they have to minify themselves (which can be difficult).

Another solution for that problem would be to use (in the templates) the non-minified JS/CSS, unless the minified files would exist. Minification would be simply a performance-enhancing factor in that case, but purely optional.

@nbraud
Copy link
Author

nbraud commented Oct 31, 2013

Regarding the implied difficulty involved in minification, I do not believe than a user who managed to pull the code from the repository would have difficulties running make. I even took care to let the user override variables from the environment in case the default doesn't matches his setup.

Regarding on-demand minimization, please re-read my previous comment, which precisely answers your remark.

@ulikoehler
Copy link

The problem is rather that most users (and Github makes it really simple to just clone a repository) won't have the closure compiler (which requires Java) / Uglify (which requires NodeJS) etc. So while running make by itself would be OK (well, only if you assume the user has make, which is not an assumption you can universally make, see e.g. Windows users), I don't like the idea of the user having to download / install / google something only to get it running, at all. You could do something like that for a large 100kSLOC+ commercial application, but ZeroBin should be easy to use and easy to install also for non-expert users. It's a nice feature that users are able to override ENV vars, but I think while it might be useful for developers, it is not useful to end-users who like to use

I, personally, would like to clone directly onto my server, where Java is not installed (for security and backup reasons), and NodeJS might not be installed either. Even if pretty much any server I'm gonna use will have make installed, that is not a general assumption you can make. Imagine the "average" Windows user, that wants to install ZB on his paid PHP webspace. You could argue that this average user might not install ZB himself, but I think a judgement about a software's userspace (which might be partially true at the moment) should never influence how easy it should be to use and setup.

I actually do know a fair amount of people who might want to install ZB on their webspace, but who don't use make all the day, and some of them don't really know what environment variables are.

I don't think your previous answer (assuming you meant #65 (comment)) really adresses my remark. Sorry if I didn't make my point clear enough before.
Let's iterate over your points and check the differences:
1) That might be true for some implementations but I (I'm not a PHP expert!) can not imagine how a single file_exists call (which returns true exactly once, when ZeroBin is started for the first time) increases the attack surface. Additionally I believe at the current state the ZeroBin core is not immune to file synchronization issues (which could lead, at least theoretically, to exploitable security issues).
2) First, ZeroBin itself (i.e. index.php) is currently not good at caching at all in its current state. So your argument rather appies. Second, if you generate a minified file (once and only once, at first startup), you got a static file. This file will be served, just as all other static files, by the webserver. If one thinks his particular setup needs additional htaccess-like settings, he can surely add whatever he wants, but is is not neccessary in any way.
3) It is in fact extremely stupid and overhead-prone, but that's exactly what I tried to adress in my previous comment. My solution is to generate a static file exactly once, at first startup.

on-demand might be misleading. As said before, I think it's a bad idea to minify it every time the minified file is needed (cached or not). However I think it's a good idea to auto-minify once (and only once!), on first startup.
It's more the demand arises from adding a <script> tag in the rendered template, and we can minify once before that script tag., than the demand arises from someone requesting the minified JS file.

@nbraud
Copy link
Author

nbraud commented Oct 31, 2013

  1. The problem is more about the minification process itself and making sure you cannot make the PHP code produce minified versions of other files.
  2. The problem is that you need, then, to somehow direct users to the newly minified file. I assumed you would do so through yet another PHP script, but it can be (partially) avoided.

The problem regarding (2) still stands, though:

  • you pay some non-trivial overhead (compared to the benefits of minification), having to verify (somehow) that the minified versions are up-to-date
  • writing the minified version must be done carefully, so as to avoid race-conditions between several concurrent users

Lastly, you seem to have lost sight of the fact that ZeroBin works perfectly well without this micro-optimization, and the added complexity of “dynamic” minification vastly outweights any foreseeable benefits.

PS: Your comment cut at “the demand arises from adding a

@ulikoehler
Copy link

Sorry for the cutoff comment, I seem to have accidentally deleted/overwritten something.

  1. If you are referring to security issues here, I assume what files need to be minified (and the name of the target files) are known at development time, i.e. you can hardcode them. It would of course be a fatal mistake to make minification files dependent on user input, but this is simply unneccessary in this particular case.
  2. No. It is actually simpler if you don't redirect (call it a workaround if you like):
    It all depends on the order you minify and render the templates.
    You only need to ensure that at the particular point in time the browser sees the <script> or <style> tag, the minified file alredy exists.
    For ZB, it is known that the browser will see the tags sometimes after you call RainTPL.
    Therefore, you just you need to place the code that will generate the static minified files before the RainTPL call, which is, conveniently, one of the last function calls in index.php.
    I.e. place the minification code at the beginning of index.php and you're ready to launch.

You are partially right about that optimization not being neccessary in some cses but let's put it that way:

  • ZB works perfectly well without minification. It might not be the fastest thing on mother earth, but there are a dozen other factors that limit the performance (e.g. the file-based IP rate limiting) for semi-high or high workloads.
  • Minification is in my experience generally a nice thing (unless you're debugging the JS), but it removes only one bottleneck.
  • ZB will not work perfectly well if users are required to use tools that are not included in the distribution and run as-is (so far, only PHP is required. You extend the dependencies) and if it therefore does not work out of the box.
  • My proposal (which is exactly that, no finished product, no code, just an idea) is just a solution that keeps minification enabled (for performance) while still keeping it dependency-free.

Again, my minification proposal is not fully dynamic.
I believe in the future the will be some PHP setup script, to setup ZB (as a more user-friendly alternative to ). You could do the minification there, if you'd like.

@nbraud
Copy link
Author

nbraud commented Oct 31, 2013

  • Yes, I am well aware that your “proposal sketch” is not fully “dynamic”.
  • You didn't address the overhead associated with checking that the minified files are up-to-date.
    This is a cost paid at each page, unlike the transfer of the JS (done one per cache-miss on the client).
  • You didn't address the fact that this would be much more complex, for dubious added value.

Moreover, I am unwilling to write additional code for this.

@ulikoehler
Copy link

  • Good to know. Let's call it pseudo-dynamic (working title).
  • I believe up-to-date-checking is a different featureset. For a basic minification implementation, I would not include that, for the sake of simplicity. It is obviously needed for simple updating, but I think the primary concern is simple installation. The problem can be solved more easily by documentating "If you're updating using git, exec git clean -xdf after updating" or something similar. It would be kinda ugly, but either that, or live with the conse
  • I don't agree at all with it being much more complex! I did never use the PHP minification before, but I believe it would boil down to something like that (I'm using placeholders):
require_once 'uglify.php';
if(!file_exists('js/min.js)) {
   minify_js('js/main.js','js/zerobin.js','js/min.js');
   minify_css('css/style.css','css/zerobin.css','css/min.css');
}

Regarding the PNG optimization, I'd prefer to check in the optimized PNGs into git. You won't debug PNGs, so I can see no reason why ZB should contain the unoptimized versions.

The value added, is IMO not dubious whatsoever. Users being able to install stuff without hard work is way more value.

I can understand that you don't simply want to extend your code and put additional work in it. However, I think in the current form it defies one of the principles of ZB I consider most important: Simple to install & dependency-free. What you did is not in any way entirely wrong or bad, but I do not want to read source code just to get something running, ever. If I'm not missing something, the neccessity of executing make is not documented in README/INSTALL/whatever.
Bottom line: Not only is it difficult (and in some respect a waste of time) to install Closure compiler / Uglify locally, you'd even have to read the Makefile to get some instructions on what you need to do to get it working.. Users should never be required to do so. This particular issue could be fixed, however, by adding some information to the README.

@nitmir
Copy link

nitmir commented Nov 1, 2013

Why do not just put the normal and minimized version inside the repository and let the maintainer / community updates the minimized version in the same time of the normal version (possibly with a script inside the repository, but useless to end users) ?

It seems both odd to me to add dependencies on install or a bunch of php code just used once .

@ulikoehler
Copy link

I think that would be a solution, there would be quite some overhead but
storage is somewhat cheap these days :)

👍

Am 01.11.2013 01:25, schrieb Valentin Samir:

Why do not just put the normal and minimized version inside the
repository and let the maintainer / community updates the minimized
version in the same time of the normal version (possibly with a script
inside the repository, but useless to end users) ?


Reply to this email directly or view it on GitHub
#65 (comment).

@Hexalyse
Copy link

Hexalyse commented Nov 1, 2013

I agree with nitmir, a simple minified version on the repo would be OK.

One thing is certain, I wouldn't like to have to install NodeJS (or worse, java) just to enable a minification feature, when ZB itself doesn't have any other dependency than PHP. That's one of the great thing about ZB, let's keep it simple.
I'd rather use an external minification tool and replace the file myself, otherwise.

@nbraud
Copy link
Author

nbraud commented May 13, 2016

Upstream is nonresponsive.

@nbraud nbraud closed this May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants