If the instructions for downloading Modernizr on the project page are followed to the letter, you're going to receive the package with the latest version which is 2.7.1. This module explicitly requires 2.6.2. The Underscore project has also skipped ahead, as well.

From the documentation for hook_libraries_info():

version: (optional) The version of the library. This should not be declared normally, as it is automatically detected (see 'version callback' below) to allow for version changes of libraries without code changes of implementing modules and to support different versions of a library simultaneously (though only one version can be installed per site).

Can we remove the 'version' keys in this hook? I am soooo eager to try this menu out but this roadblock cost me a good hour.

Thanks!

(Patch to follow)

CommentFileSizeAuthor
#11 status-report-2.png40.71 KBjessebeach
#11 Status_report___Drupal_D7_Dev.png40.71 KBjessebeach
#3 navbar-remove_version_req-2155461-2.patch2.75 KBothermachines
#1 navbar-remove_version_req-2155461.patch913 bytesothermachines

Comments

othermachines’s picture

othermachines commented
Status: Active » Needs review
StatusFileSize
new navbar-remove_version_req-2155461.patch913 bytes

Patch to remove JS library version dependencies in hook_libraries_info().

othermachines’s picture

othermachines commented
Status: Needs review » Needs work

Darn. Apparently I acted too soon. hook_library requires 'version' although hook_libraries_info() doesn't. Will need to investigate further.

It would be nice if there were a way to specify a *minimum* version. Having to specify the exact numbered version seems really limiting. Also high maintenance.

Anyone?

othermachines’s picture

othermachines commented
Status: Needs work » Needs review
StatusFileSize
new navbar-remove_version_req-2155461-2.patch2.75 KB

OK, let's try this again...

Spent quite a bit of time looking into best practices (and common practices where this wasn't clear). In every case I could find, a version is specified only when the library is packaged with the module (or with core, as is the case with jquery.js, etc.). In the case where people are responsible for acquiring the package themselves this can turn into a real headache for everyone, including (especially?) maintainers.

These "bundled" libraries are generally the only ones registered with hook_library(), as well - at least in 7.x. But I may open a separate issue for that.

The second patch parses the version number from the file itself, so we no longer require a version, but a string that matches the given pattern. It also requires the minified version. This is the technique that colorbox uses (see it here).

hook_libraries_info() has a couple of unfortunate limitations:

  • We can't pattern-match a version on a variant (apparently some but not not all top-level properties are allowed). This is why I've removed variants altogether. In the colorbox example, a 'source' and 'minified' are specified as variants but because of the version callback it remains dependent on the minified version (removing it would result in nothing loading).

    That said, I think it is easier to tell people to download the minified file than to mess with version numbers. (I'll probably submit a fix for this over at Libraries, so at some point we may be able to accommodate both.)

  • We have to provide a version as a string - either hard-coded or parsed from the file. The docs say no, that it is automatically detected, but if neither 'version' nor 'version callback' are provided it just won't load the file (Issue #2046919).

Anyway, this is all tested and working with my set-up - yay! (Modernizr 2.7.1 and, Underscore 1.5.0, Backbone 1.0.0) I love it so far. We'll be installing it on our own site and testing it rigorously in production in the hopes that we'll be able to let our clients install it in advance of Drupal 8.

Cheers -

hass’s picture

hass commented
Status: Needs review » Needs work

Looks incorrect based on lib api docs and was just changed a few weeks ago to allow non min versions for debugging. You remove all this.

othermachines’s picture

othermachines commented

Looks incorrect? How do you mean?

My main objective here (as should be clear from my very extensive explanation) is to make the module accessible to users setting up for the first time. I get the sense from the issue queue that people are frustrated, and I know some of the reasons why because that was me last week:

1) module's version requirements are outdated
2) documentation is not complete
3) what documentation is there leads to downloading a library version that will ultimately rejected by the module

I know that sometimes these things are easy to overlook after you are already fully immersed in a module's development.

I also understand that it would be ideal to allow both full and minified versions. It was a compromise that I fully explained above.

If I'm wrong, I'm wrong, but I think this issue is at least worthy of some discussion.

othermachines’s picture

othermachines commented

OK, so I think I've managed to whittle this down to the most important points. Hopefully this will make things easier!

  • With the existing code in navbar_libraries_info(), the minified versions will never be loaded. I really don't think this is understood by everyone (although @jessebeach specifically points to the problem here).

  • The 'version' property does not actually do a version check at all. It's really nothing more than a placeholder property. (This was news to me.)

  • As far as I can tell - and I have tested A LOT of different variations - there is no way to provide an automatic fallback to 'x' variant. The 'variants' property is just a place to define other versions that would still need to be explicitly requested in libraries_load().

  • colorbox gets around this by providing a system configuration setting that allows you to select whether you prefer the compressed (minified) or uncompressed version.

Discussion points:

  1. Consider providing a configuration option in the UI so that people could select which version they want to use. This version should default to minified since the source version should only be loaded for debugging purposes.

    The alternative would be to explicitly ask for the 'source' variant in hook_library() if devel module is enabled, but of course this would result in confusing errors if there is no source version available.

  2. Even though 'version' property doesn't do much of anything, the API docs say 'version' property should not be declared normally. We should provide 'version callback' instead.

  3. Once this is resolved, better documentation should be provided. Working on this over here but obviously needs to be put on hold for now.

The big goal, here, is to get fewer issues such as these: Modernizr library: modernizr.js file or modernizr.min.js? (posted just two days ago). I'll do what I can to help, but some real feedback would be great.

Cheers -

othermachines’s picture

othermachines commented
Title: Remove version requirement for javascript libraries » Issues with version and variants (full vs min) when loading libraries

Changing title

jessebeach’s picture

jessebeach commented

Thank you for the thorough and detailed investigation othermachines. This is my 'big project' for the day tomorrow to sort out. It's the lot bit of troublesome behavior before we can cut a release of Navbar.

othermachines’s picture

othermachines commented

Sounds good. I can patch in the UI config option if you decide to go that route. I'm off this week.

jessebeach’s picture

jessebeach commented

So, after reading through the Libraries' module code for a few hours, here are my conclusions:

  • I can't grok that module. It has baked into it a workflow that probably functions well for the author, but for anyone else is just obtuse.
  • I don't want to sink hours into creating the "perfect" implementation of the Libraries module API for the sake of completeness.
  • I'd rather understand what kind of development and debugging we need for this module at a basic level and just provide that.
  • We should just always load the minified versions of the libraries. Chrome will expand the minified versions onto separate lines in the Source tab and this works for most debugging purposes. If more is needed, you can always copy-paste the source code into the minified file for a hackish-and-temporary debug session.

I'll work the minimum version info for the libraries into the hook_requirements implementation as well as better error messages for e.g. the required file name pattern for each one.

jessebeach’s picture

jessebeach commented
Status: Needs work » Fixed
StatusFileSize
new Status_report___Drupal_D7_Dev.png40.71 KB
new status-report-2.png40.71 KB

I committed an update to our libraries implementation here in 7f697361 and (small fix) ef1f4974.

Navbar now declares minimum library versions and variants within that version -- source and minified. We're loading the minified by default. othermachines, I gave you a "hook" here to load the source by setting a variable with variable_set: navbar_libraries_variants. The variant is keyed by library, e.g.

variable_set('navbar_libraries_variants', array(
  'modernizr' => 'source',
  'backbone' => 'source',
  'underscore' => 'minified',
));

So if you want to propose configuration to set these values, you have a way to change them.

Here's an example of the Status page where the Modernizr library that is installed is version 2.5.2, but version 2.6.2 or greater is required.

hass’s picture

hass commented

Looks like these are the commits:

http://drupalcode.org/project/navbar.git/commit/7f69736
http://drupalcode.org/project/navbar.git/commit/e25473d

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.