Change bundle path and fix relative URL's #41

rehandalal · 2013-02-24T04:59:50Z

Feel free to say: "THESE ARE USELESS CHANGES AND NOBODY NEEDS THEM!".

This does a couple of things:

CSS bundles will now be in ~~MEDIA_PATH/bundled/css~~ MEDIA_PATH/BUNDLES_DIR/css
JS bundles will now be in ~~MEDIA_PATH/bundled/js~~ MEDIA_PATH/BUNDLES_DIR/js
Relative URL's in CSS will now be fixed to point to the correct URL when bundled. (This was a problem on SUMO and recently on Input)

r?

rehandalal · 2013-02-24T06:02:19Z

I should also probably point out the reason for bundling to the bundled folder.

I happened to have a bundle called jquery and sadly one of the files in the bundle was jquery-min.js which (you guessed it) got replaced with the bundle file.

Now admittedly, this is a silly edge case that would normally not be an issue, and is pretty easy to get around. But I generally like the idea of tossing bundled files in another folder because it also keeps file clutter down in my js/css folders.

mythmon · 2013-02-24T06:16:00Z

jingo_minify/management/commands/compress_assets.py

+
+        parse = lambda url: self._fix_urls_regex(url, relpath)
+
+        css_parsed = re.sub('url\(([^)]*?)\)', parse, css_content)


This should be a raw string, like this

css_parsed = re.sub(r'url\(([^)]*?)\)', parse, css_content)

Otherwise, python and re get confused about the back slashes.

willkg · 2013-02-24T14:49:50Z

If someone updates to this, will they be hosed? Are there things they need to do to update to this version?

cvan · 2013-02-24T20:04:47Z

jingo_minify/management/commands/compress_assets.py

@@ -34,6 +34,7 @@ class Command(BaseCommand):  # pragma: no cover
    minify_skipped = 0
    cmd_errors = False
    ext_media_path = os.path.join(get_media_root(), 'external')
+    bundles_dir = getattr(settings, 'BUNDLES_DIR', 'bundled')


is this not in get_media_root()?

if I decide to not set settings.BUNDLES_DIR can we not do the bundled/ stuff?

sure i could default that to .

rehandalal · 2013-02-25T08:34:15Z

@willkg with the new change to the default value of BUNDLES_DIR there should be no compatibility issues.

rehandalal · 2013-03-07T02:44:54Z

Test added ^

rehandalal · 2013-03-07T02:56:08Z

And now tests pass ^

If this is all good I'll squash and push

jsocol · 2013-10-28T22:19:59Z

@cvan any outstanding comments here? @rehandalal Still want to get this merged in?

davidbgk · 2014-04-09T14:31:33Z

requirements.txt

@@ -3,3 +3,4 @@ jingo==0.4
 Fabric==1.4.3
 -e git://github.com/jbalogh/django-nose.git@83c7867c3f90ff3c7c7471716da91b643e8b2c01#egg=django_nose-dev
 mock==1.0b1
+GitPython==0.1.7


I can't find where you use that package in the diff.

It's unrelated but missing. There's a test in #63 that demonstrates the issue. Any new tests that hit that area of code will trip over it.

davidbgk · 2014-04-09T14:34:41Z

I can't understand why there is the /bundled/ path still hard-coded in some places given that there is a setting for that path.

davidbgk · 2014-04-09T15:54:16Z

jingo_minify/management/commands/compress_assets.py

+        """Run over the regex; Fix relative URL's"""
+        url = url.group(1).strip('"\'')
+        if url.startswith(('data:', 'http:', 'https:')):
+            return url


Shouldn't that one be wrapped in url() too?

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

rehandalal · 2014-11-01T17:55:57Z

I just did the following:

Rebased/squashed the original commits in this pull request and fixed all the conflicts
Updated the tests to be more thorough

I still think this is pretty useful!

r?

rehandalal · 2014-11-11T07:12:48Z

@jsocol think you could have a quick look-see at this? THANKS!

jsocol · 2014-11-11T19:45:34Z

I can but not for another day or so, and I don't have the context that @cvan would

cvan · 2014-11-12T23:30:44Z

jingo_minify/helpers.py

-        return (_get_item_path('js/%s-min.js?build=%s' % (bundle, build_id,)),)
+        item = 'js/%s-min.js?build=%s' % (bundle, build_id,)
+        item = os.path.join(getattr(settings, 'BUNDLES_DIR', ''), item)
+        return (_get_item_path(item),)


why again do we return a tuple of one item?

cvan · 2014-11-12T23:39:14Z

looking good. sorry it's taken so long to get this in. I don't think we've understood the use case here, hence the delays.

rehandalal · 2014-11-12T23:56:06Z

I'll clean up the code.

For me, this is why I made the changes:

Having a special directory that bundles get stored in makes it easier for me to filter those files out through .gitignore. I also had one edge case where I had a file which shared the same name as the final minified bundle that it belonged to which caused a whole mess. Admittedly that's easily fixed by renaming the file or the bundle but I prefer this.
Fixing the relative URLs helps when you're bundling files that are installed by bower and they have additional image assets that are linked to such as FancyBox. Without this, sometimes I have to copy these image files into the css folder or other places and that was just getting messy.

rehandalal · 2014-12-04T00:14:44Z

@cvan fixed based on feedback ^

mythmon reviewed Feb 24, 2013
View reviewed changes

cvan reviewed Feb 24, 2013
View reviewed changes

davidbgk reviewed Apr 9, 2014
View reviewed changes

davidbgk mentioned this pull request Apr 9, 2014

Fix broken images (bug 990054) mozilla/addons-server#36

Merged

davidbgk reviewed Apr 9, 2014
View reviewed changes

dean added a commit to mozilla/kitsune that referenced this pull request Sep 3, 2014

[bug 1035319] Temporary fix for jingo-minify.

334a6ef

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

dean added a commit to dean/kitsune that referenced this pull request Sep 3, 2014

[bug 1035319] Temporary fix for jingo-minify.

70f0a13

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

dean mentioned this pull request Sep 3, 2014

[bug 1035319] Switching from /vendor to peep. mozilla/kitsune#2083

Closed

dean added a commit to dean/kitsune that referenced this pull request Sep 4, 2014

[bug 1035319] Temporary fix for jingo-minify.

241ee8f

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

dean added a commit to dean/kitsune that referenced this pull request Sep 9, 2014

[bug 1035319] Temporary fix for jingo-minify.

bdc4151

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

dean added a commit to mozilla/kitsune that referenced this pull request Sep 17, 2014

[bug 1035319] Temporary fix for jingo-minify.

c6ca927

This needs to be updated after the PR is merged into master at: jsocol/jingo-minify#41

rehandalal added 2 commits November 1, 2014 13:33

Change bundle path and fix relative URL's

c08bdc4

update tests

0441226

revised README.rst

0df6315

cvan reviewed Nov 12, 2014
View reviewed changes

feedback

0fe93c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change bundle path and fix relative URL's #41

Change bundle path and fix relative URL's #41

rehandalal commented Feb 24, 2013

rehandalal commented Feb 24, 2013

mythmon Feb 24, 2013

willkg commented Feb 24, 2013

cvan Feb 24, 2013

cvan Feb 24, 2013

rehandalal Feb 25, 2013

rehandalal commented Feb 25, 2013

rehandalal commented Mar 7, 2013

rehandalal commented Mar 7, 2013

jsocol commented Oct 28, 2013

davidbgk Apr 9, 2014

jsocol Apr 9, 2014

davidbgk commented Apr 9, 2014

davidbgk Apr 9, 2014

rehandalal commented Nov 1, 2014

rehandalal commented Nov 11, 2014

jsocol commented Nov 11, 2014

cvan Nov 12, 2014

cvan commented Nov 12, 2014

rehandalal commented Nov 12, 2014

rehandalal commented Dec 4, 2014


		parse = lambda url: self._fix_urls_regex(url, relpath)

		css_parsed = re.sub('url\(([^)]*?)\)', parse, css_content)

Change bundle path and fix relative URL's #41

Are you sure you want to change the base?

Change bundle path and fix relative URL's #41

Conversation

rehandalal commented Feb 24, 2013

rehandalal commented Feb 24, 2013

Choose a reason for hiding this comment

willkg commented Feb 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rehandalal commented Feb 25, 2013

rehandalal commented Mar 7, 2013

rehandalal commented Mar 7, 2013

jsocol commented Oct 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbgk commented Apr 9, 2014

Choose a reason for hiding this comment

rehandalal commented Nov 1, 2014

rehandalal commented Nov 11, 2014

jsocol commented Nov 11, 2014

Choose a reason for hiding this comment

cvan commented Nov 12, 2014

rehandalal commented Nov 12, 2014

rehandalal commented Dec 4, 2014