TomodomoCo / total-slider

A WordPress plugin for creating, editing and removing ‘slides’ with text and images (for a homepage, for example).
GNU General Public License v2.0
7 stars 2 forks source link

Adopt the .min.js convention #26

Closed chrisvanpatten closed 12 years ago

chrisvanpatten commented 12 years ago

Using .min.js to signify a minified JavaScript file is a de-facto standard, and is now being embraced by WordPress core as well. We should follow suit, and switch from .dev.js and .js to the .js and .min.js approach as well.

chrisvanpatten commented 12 years ago

Currently, themes are unable to use the .min.js convention.

I'm not sure how we can get around this while maximising theme compatibility (is it a question of asking themes to directly enqueue their JavaScript file instead of making assumptions? I tend to think specificity is the mother of sanity) but I think it's worth re-opening the issue for 1.1.1 examination.

chrisvanpatten commented 12 years ago

I see the problem now.

The admin panel looks for the .js version, and then live site looks for the .min.js version.

I'm going to file a pull request that has the admin site look for the .min.js version.

I still think a method where the theme can define its JS file may be useful, but is definitely a question for a future release.

This is kinda beyond me. It looks like this function is to blame; it throws an exception things if the unminified JS isn't available. I think this will definitely frustrate template developers, as it requires them to maintain two versions of their JS, essentially.

I think a better, saner method would be this:

If SCRIPT_DEBUG is FALSE or unset:

  1. Look for themename.min.js first.
  2. If themename.min.js exists, exit happily.
  3. If themename.min.js does not exist, look for themename.js.
  4. If themename.js does not exist, throw an exception.

If SCRIPT_DEBUG is TRUE:

  1. If using themename.min.js as the main script, look for themename.js.
  2. If it does not exist, silently log an error, and continue to use themename.min.js to minimize breakage.
    • We can encourage theme devs using the .min.js convention to bundle unminified scripts, but ultimately can't force them. This prevents any errors there.
  3. If themename.js does exist (or if using it as the main script), use it.

Does that process make sense?

chrisvanpatten commented 12 years ago

I've reassigned this to 1.1. I realize it seems like a fairly large change given our deadline, but seeing as this release is about the template architecture, I think it's absolutely crucial that we nail it the first time around, or else we'll be setting template developers up for frustration.

PeterUpfold commented 12 years ago

Just to double-check -- do you have SCRIPT_DEBUG off? If that remains on, the .js (formerly .dev.js) will always be preferred to the .min.js.

chrisvanpatten commented 12 years ago

I do have SCRIPT_DEBUG off. This definitely appears to be a logic issue in canonicalize() (full code here, excerpted below) —

...

$jsExists = @file_exists($pathPrefix . $this->slug . '/' . $this->slug . '.js' );
$jsMinExists = @file_exists($pathPrefix . $this->slug . '/' . $this->slug . '.min.js' );

...

else if (!$jsExists)
{
    $missingFile = 'JS';
    $expectedLocation = $pathPrefix . $this->slug . '/' . $this->slug . '.js';                              
}

...

It's looking for the unminified JS in this else if, and throwing the exception before it gets a chance to use the unminified code.

PeterUpfold commented 12 years ago

This is a misunderstanding on my part.

It seems illogical to me to do it this way, but you’re suggesting that theme developers will use the .min.js version as the canonical script, and somehow un-minify it for debugging purposes? (Wouldn’t you always want to keep source files as well as ‘compiled’ files?)

Today, TS is expecting the .js file to absolutely be there, and the .min.js is seen as a neat bonus for running things a little faster.

Clearly, if the opposite behaviour is the that which theme devs expect, that is the way around that TS should do it. I suppose this actually mirrors how it was before — .dev.js was a ‘nice to have’ version for easier debugging, and .js was the canonical file.

I still do want to send out the message that we strongly encourage shipping both. In fact, the source JavaScript, as the “preferred form” for editing, is something that has a GPL angle to it too. (Not legal advice, kids.)

chrisvanpatten commented 12 years ago

I think the logic I laid out above makes the most sense to satisfy all parties. .min.js is ultimately a neat bonus, but I think the current arrangement, where one script is requested in one place and the other is requested in another place, is going to lead to more confusion and misunderstanding than it's worth.

I definitely agree that we want unminified scripts shipped with templates (and it should be a requirement for all officially "endorsed" themes in a future template gallery) but I think this is a separate issue. It just seems confusing to get a "you don't have a JS file" script when the template clearly does have a .min.js file available that would work perfectly well.

PeterUpfold commented 12 years ago

This has been fixed by 1a5f163.