Skip to content

Switch from jingo-minify to django-pipeline#2401

Merged
rehandalal merged 11 commits into
mozilla:masterfrom
rehandalal:pipeline
Mar 24, 2015
Merged

Switch from jingo-minify to django-pipeline#2401
rehandalal merged 11 commits into
mozilla:masterfrom
rehandalal:pipeline

Conversation

@rehandalal
Copy link
Copy Markdown
Contributor

I've been exploring switching to Bower to handle all our front end dependencies and one of the first issues we run into is that jingo minify needs to land my changes that fix relative url's in CSS files. Since that pull has been opened for a few years and the general maintenance of jingo minify is not the greatest I decided switching to django-pipeline was a better option. The switch was way simpler than I expected.

Some things to note:

  • settings_local.py will need to be updated on each of the server before we deploy this and then probably again to remove the jingo-minify stuff once we have finished deploying/testing this.
  • We are switching from cleancss to cssmin since I didn't feel like writing a django-pipeline compiler for cleancss.

r?

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

Funny this--@mythmon and I were just talking about switching to gulp for everything or something like that.

@rehandalal
Copy link
Copy Markdown
Contributor Author

/me nods. that could be interesting, but i think that would likely be a much larger change. i think while we figure out what that looks like this is a pretty good change. it already cuts out the double-refresh thing that i find super annoying while developing.

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

OMG. I hate the double-hard-refresh thing.

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

This is failing the Travis thing because it can't find lessc. Just as an eff why eye.

@rehandalal
Copy link
Copy Markdown
Contributor Author

@willkg it was but i fixed that in my second commit. the qunit tests page was broken but i just fixed it ^

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

Just as an eff why eye, I'm so glad you're doing this. I'm so going to do this for fjord, too.

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

Also, I'm just chatting right now. I can review this on Monday (i.e. 24 hours from now) if other people don't get to it first.

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 8, 2015

I think we convert this PR into a sumodev message board. 👍

@rehandalal
Copy link
Copy Markdown
Contributor Author

haha! i was actually just about to do this for fjord!

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 9, 2015

Ditching WTFinder is totally unacceptable. r-

Comment thread scripts/update/deploy.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't do this at compile time, does it happen at request time? Or does collectstatic now do the compress_assets part?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collectstatic handles everything

@mythmon
Copy link
Copy Markdown
Contributor

mythmon commented Mar 9, 2015

Looking at pipeline, I like it. It seems to have pluggable compilers and minifiers, so that's really cool. It has an es6 compiler that uses Babel, which I'm a big fan of, and in particular Babel supports the jsx compilation I'm going to need to do for my React stuff. This makes me much less gung-ho about using Gulp for all this compilation stuff, since pipeline seems to be able to handle it all.

All these changes look fine, I have a small question about when compilation happens, but besides that, yay! The tests are certainly a problem, but I bet that is fixable.

@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 11, 2015

So... I spent some time figuring out what was going on with the tests.

On my machine, kitsune tests take:

  • 4m46s: master branch
  • 22m: the changes in this PR

22m is crazy. I want to do this for Input, too, because I like jingo-minify like I like ham and cheese sandwiches drenched in used motor oil so I spent some time looking into it.

After some experimentation, I discovered that if PIPELINE_ENABLED = False, then the jinja tag triggers compiling of every file for every test. If we change that to PIPELINE_ENABLED = True, then this doesn't happen. Code in question is here (we're using 1.4.6):

https://github.com/cyberdelia/django-pipeline/blob/4c678d2fe033ae1dc23f552462cd382b399ee5dc/pipeline/templatetags/pipeline.py#L36

This sort of makes sense. PIPELINE_ENABLED defaults to not DEBUG. Django runs the tests with DEBUG = False. By that logic, seems like we should be running with PIPELINE_ENABLED = True. I think we need to explicitly set PIPELINE_ENABLED = True in settings_test.py because of the way it gets the default value:

https://github.com/cyberdelia/django-pipeline/blob/4c678d2fe033ae1dc23f552462cd382b399ee5dc/pipeline/conf.py#L9

Pretty sure that's the only environment where it could potentially pick up the wrong value.

I didn't know whether this means we have to run collectstatic before a test run or not (we do for Input (which is using jingo-minify)). I removed static/ and ran the tests and didn't have problems. Ergo, I conclude we don't have to run collectstatic before running the test suite.

Also, on a positive note, the tests with this branch and PIPELINE_ENABLED = True run in 4m on my machine--that's 46s faster. So that's nice.

And that's all I have to say about that.

@rehandalal
Copy link
Copy Markdown
Contributor Author

updated ^

/me crosses finger.

@rehandalal
Copy link
Copy Markdown
Contributor Author

tests pass in a normal amount of time. thanks for looking into that @willkg!

r?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be just jqueryui?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good spot!

@rehandalal
Copy link
Copy Markdown
Contributor Author

fixed ^

@mythmon
Copy link
Copy Markdown
Contributor

mythmon commented Mar 11, 2015

The changes look good. I'm going to test it out.

@mythmon
Copy link
Copy Markdown
Contributor

mythmon commented Mar 11, 2015

This works for me. Lets see how it does on stage. r+

@mythmon
Copy link
Copy Markdown
Contributor

mythmon commented Mar 23, 2015

I deployed 5df4075 to stage, and checked that it worked with cache busting. It did, so that's cool. It also cache busted a bunch of our other assets, including a sprite sheet we were manually cache busting. Nice! I think this is ready to go.

rehandalal added a commit that referenced this pull request Mar 24, 2015
Switch from jingo-minify to django-pipeline
@rehandalal rehandalal merged commit 8fd7ba6 into mozilla:master Mar 24, 2015
@willkg
Copy link
Copy Markdown
Member

willkg commented Mar 24, 2015

WOO HOO!!!!111!!!11!! 🎈 👍 🎆

@rehandalal rehandalal deleted the pipeline branch March 31, 2015 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants