canton7 / fuelphp-casset

Better asset management library for fuelphp (with minification!)
MIT License
103 stars 29 forks source link

Inline JS as dependancy #15

Closed antitoxic closed 12 years ago

antitoxic commented 12 years ago

Use case

Hello, I'm trying to implement HTML5 boilerplate layout with Casset asset management.

In the html5 boilerplate template, jQuery framework is first atempted to be loaded from google api. Then there's a 1 line inline script that checks whether jQuery was successfully loaded and if not loads the local copy.

The best i could think of was:

<?php echo Casset::render_js('jquery')?>
<script>window.jQuery || document.write('<?php echo trim(str_replace('</script>','<\\/script>',Casset::render_js('jquery_fallback')))?>')</script>

NOTE: I use:

Casset::render_js('jquery_fallback')

instead of:

Casset::get_filepath_js('jquery.1.7.1.js')

because there is no way to pass the file as array to Casset::get_filepath_js (i.e. Casset::get_filepath_js(array('jquery.1.7.1.js', 'jquery.1.7.1.min.js')) which make the file one and the same for all environments.

The problem

I've made a separate issue for get_filepath_js() (#14) so the problem for this lies only in: Inability to add inline JS as a member of a group - currently every script in a group must be a file location.

Proposal

There should be file-specific options where the developer can pass something like array('script'=>true) to indicate the file is an inline script. I can think of of two solutions where of course other can exist.

a) Support only runtime definition

This require fix for issue #14 (get_filepath_js)

Casset::js(
    sprintf('window.jQuery || document.write(\'<script src="%s"><\\/script>\')', Casset::get_filepath_js(array('lib/jquery-1.7.1.js', 'lib/jquery-1.7.1.min.js'))),
    array('script'=> true),
    'jquery'
);

b) Support for declarations in casset.php

The script indicator which currently can only be the file location to support array representation Also additional option filter is required which accepts a format key which format the inline script with sprintf.

array(
    'window.jQuery || document.write(\'<script src="%s"><\\/script>\')',
    array('script'=> true,
          'filter'=> array(
              'format'=> array(
                  array('lib/jquery-1.7.1.js', 'lib/jquery-1.7.1.min.js')
              )
          )
    )
),
canton7 commented 12 years ago

Righty, I've sent a while pondering over this, and I've come up with a few points:

  1. get_filepath_* are for when you want to circumvent most of Casset's processing. This isn't your intention here, so I think that get_filepath_* is the wrong function for the job.
  2. There's no real advantage (correct me if I'm wrong) to using get_filepath_js('jquery.1.7.1.js') over simply hard-coding the path. (Unless the capabilities of get_filepath_* were augmented, but that's another issue). In both cases you have to hard-code the jquery version, and the only advantage to using get_filepath_* is that Casset works out the correct path for you, which is unlikely to change anyway.
  3. Therefore, either you don't really want any involvement from Casset (in which case you might as well hard-code the path), or you want to use Casset to centralise the definition of which jquery version to use, to handle minifying/combining, etc, in which case render_* is the right function to be using.
  4. If you want to follow the latter route, my preferred resolution (building on #14) would be to allow you to tell render_* not to produce tags. This would allow you to do the following:
<?php echo Casset::render_js('jquery')?>
<script>window.jQuery || document.write('<script src="<?php echo Casset::render_js('jquery_local', array('gen_tags' => false)) ?>"></script>

When it comes to adding slightly obscure functionality to Casset, I'm very much in favour of giving the end-user the tools to work with, and letting them get the job done their way. In this way, we don't end up coding for a thousand slightly different but equally obscure use-cases. This can be seen in the way that parsing files (e.g. for SASS),and cache-busting, are both handled by callbacks, rather than by some more specialised system.

This makes me very much against option b). You can achieve exactly the same end-result, in less code and with more flexibility, by simply retrieving the asset URL from Casset's render_js, and constructing the <script> tag yourself.

What do you think?

antitoxic commented 12 years ago
<?php echo Casset::render_js('jquery_local', array('gen_tags' => false)) ?>

can suffice. However I was trying to expose the frontend developer to as little php as possible. I mean in practice what I'm trying to achieve is an optional javascript inlude.

The entire window.jQuery || .... thing is basically checking whether previous s include succeeded. So in a way it is a dependancy resolver.

If Casset has the goal to solve asset dependancies then an option to provide fallback is a must.

At least I think so :) Tell me if this doesn't make sense.

canton7 commented 12 years ago

Hmm, on the other hand, Casset runs under the assumption that all asset files are present (it will start throwing exceptions if they aren't).

So yes, while I see your point, I'm not entirely convinced that this is Casset's domain. Indeed, neither of your suggestions directly relate to dependency resolution -- rather to ways of customising the <script> tag which Casset spits out, in order to make javascript do its dependency resolution.

This is a separate problem, and one which can be solved in two ways: by providing the user with enough "raw" data that he can construct his own tags (the 'gen_tags' solution), or by providing a means for the user to configure how Casset creates tags.

We've discussed the former. Re the latter, I'd prefer the concept of allowing the user to create a callback, which is called when Casset generates the tag. I much prefer this to forcing the user to provide some sprintf-able string at configure-time, although it's not nearly as useful as the other two callbacks (as the same result can be achieved by other means).

Does that make sense?

antitoxic commented 12 years ago

Yes :)

I'm not providing a solution as I don't have one :) I tried but I could only think of this or something in the lines of yepnope.js which will check a condition and if true insert another asset. - But hey this is ridiculous as in the case of not found asset the 404 will be evaluated by the client browser and not php.

My stupidity - Hence the issue report. :)

I will gladly use the 'gen_tags' solution.

canton7 commented 12 years ago

Cool.

Of course, there's nothing to stop you writing a little helper function to shift the PHP away from the view, and out of reach of your frontend developer.

Creating a callback won't be hard, and I might well do it if I get bored. For now, though, if 'gen_tags' is good enough for you, I'll close this issue and fix #14.

canton7 commented 12 years ago

I'm closing this, as #14 has been fixed, which was the agreed solution.