TomodomoCo/total-slider

Adopt the .min.js convention

Closed this issue · 8 comments

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.

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.

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?

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.

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.

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.

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.)

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.

This has been fixed by 1a5f163.