I'm using the rather wonderful grunt-modernizr package for Grunt.js to automatically determine my modernizr custom build requirements *and* download my custom build for me. I have this all working nicely, with the custom build being placed in libraries/modernizr for your module to then use.
The bugger is, the downloaded custom build contains no version number as your module expects it (example attached as .txt). This results in Status report reporting that Modernizr is not installed.
If you search within the attached file for "2.7.1" you'll notice that there is a version number included, albeit a bit more obscurely than any of us would like.
2 questions:
1) Does this prevent the Modernizr module from using the library completely or is it just a minor reporting issue?
2) Can you propose way to fix this?
Comment | File | Size | Author |
---|---|---|---|
#14 | 2222253-14-regex.patch | 880 bytes | rupl |
modernizr.min_.js_.txt | 9.31 KB | dddbbb |
Comments
Comment #1
dddbbb CreditAttribution: dddbbb commented1) It seems this does prevent the module from using the library at all which is frustrating.
Comment #2
ruplThanks for the bug report! Also thanks for including your whole Modernizr file, it helped me reproduce the issue and start debugging.
Yes, the reporting error seems to be just a surface issue. However at the same time this does seem to be preventing its inclusion on the page, which is bizarre and frustrating to me too.
I thought this would be something simple but unfortunately it is evading me at the moment. Can you just add the version number into your file for now? I downloaded your file and when I manually added the version number it immediately starts working. I'll work on a fix in the meantime, but I can't figure it out right now.
Comment #3
ruplCan you give this patch a try to see if the reporting error is fixed?
Comment #4
dddbbb CreditAttribution: dddbbb commentedYes, I'd already reached the same conclusion about manually adding the version number for the time being. That seems to do the trick for now while we figure out a better fix.
I'm also attempting to tackle this from the other end:
https://github.com/Modernizr/grunt-modernizr/issues/75
Either way, I still think that the Modernizr module should not fail to load the entire lib just because of a missing version number.
Using
r="([\d\.]*)"
on my attached custom build seems to get me part of the way there but my RegX & PHP just aren't good enough to give you a patch worth submitting.Comment #5
dddbbb CreditAttribution: dddbbb commentedRight-O. Will give the patch a whirl and report back...
Comment #6
dddbbb CreditAttribution: dddbbb commentedOops, didn't mean to delete the patch.
Comment #7
dddbbb CreditAttribution: dddbbb commentedYes, the reporting error is fixed with the patch in #3 but the library still fails to load if the version number isn't in the comment at the top of the custom build.
Comment #8
dddbbb CreditAttribution: dddbbb commentedComment #9
ruplmy beautiful patch!!!! haha just kidding.
I agree the module is doing us wrong at the moment. It's a bug, it happens. My PHP is none too sharp these days either. But this is a show-stopper for fully integrating grunt-modernizr functionality as a Drush command so I want this fixed as bad as you do. We'll figure something out.
I committed the reporting error fix to dev. Thanks for testing.
Comment #10
dddbbb CreditAttribution: dddbbb commentedThe following pattern is probably a little more robust for singling out the version number from the code (assuming that the variable name "r" is consistent):
var r="([\d\.]*)"
In my attached custom build this will return
var r="2.7.1"
- we'd then just need to create a sub string and strip off the surrounding cruft.Could we attempt to grab the version number from the commented code first (as the module currently does) but then if it fails, try to get the version number from the JavaScript (along the lines of the RegX I've quoted above)? Then if both attempts fail, print "Version unknown" in the Status report and don't load the library. Thoughts?
Comment #11
dddbbb CreditAttribution: dddbbb commentedDidn't mean to change status. We keep posting at the same time!
Comment #12
dddbbb CreditAttribution: dddbbb commentedI've just chanced upon grunt-text-replace and now have a basic grunt task for getting around this version number business in the short term.
Once you have grunt-text-replace installed, add the following task to your Gruntfile.js (make sure it's run after grunt-modernizr):
That does a rather slap dash job of adding in a totally made up version number to the modernizr build created by grunt-modernizr just to keep this module happy in the meantime. Obviously you'll want to tweak the path to your modernizr build to match your setup.
Comment #13
ruplClever, thanks for the update! I still can't figure out why this is happening inside the module.
Comment #14
ruplok, I think I got this! It's a tiny patch but I was able to use your Modernizr build
in comment 1in the OP and it both reports the correct version and successfully prints the<script>
tag.Comment #15
ruplBTW, could you share your thoughts on integrating with grunt-modernizr in this issue? #2239291: Integrate with grunt-modernizr
Comment #17
ruplI made some other decisions based on news from the yepnope team that they have deprecated their code. I'll keep the discussion in the other thread: #2311603: Address policy on upstream support
However, this means we can be a little more strict with our regex and I am just hardcoding it to look for a Semver starting with major version 2 (
2.x.y
). As an aside, that uglified variable name (r=""
) seems to change depending on the build, so just checking for the number is more reliable.Sooo.. I have committed a fix to dev. Thanks again for reporting this and helping me brainstorm it.
Comment #18
dddbbb CreditAttribution: dddbbb commentedExcellent news - glad I could help :)