Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Build JavaScript using Gulp and install some JavaScript libraries with Bower #3041

Closed

Conversation

openjck
Copy link
Contributor

@openjck openjck commented Jan 30, 2015

The more testing we can do, the better. This changes a lot of important resources.

Some things to think about:

  • This ends the use of jingo-minify for building JavaScript assets. Has it been replaced completely? Have all of the {{ js() }} calls been replaced? Aside from the {{ js() }} calls, is there anything else that needs to be updated?
  • Are JavaScript-powered features still working well?
  • Are we on board with this strategy of committing generated files (like minified/concatenated JavaScript) but not the tools that generated them (like Gulp plugins and Bower packages)? Do we like how the approach was put into practice (using gulp watch in foreman)?

Other notes:

  • You'll need to vagrant provision before reviewing this to get the latest copy of ~/.bash_profile in the VM

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 2 times, most recently from 17e1e08 to 9bae013 Compare January 30, 2015 18:37
@@ -0,0 +1,15 @@
{
"name": "MDN",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you name that kuma as that's the name of the software while MDN is the product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 7 times, most recently from d54d5a7 to 532f04b Compare January 30, 2015 21:26
@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 2 times, most recently from c9e5b47 to fc867c7 Compare February 3, 2015 19:37
@openjck openjck changed the title Build JavaScript using Gulp and install jQuery with Bower Build JavaScript using Gulp and install some JavaScript libraries with Bower Feb 3, 2015
@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 6 times, most recently from fec2e47 to 040b03d Compare February 4, 2015 17:49
@openjck
Copy link
Contributor Author

openjck commented Feb 4, 2015

I was adding commits to this PR yesterday and today, but I'll stop now so that we can review something more stable. This is ready to be reviewed whenever.

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 3 times, most recently from 49753a6 to cb48cc0 Compare February 4, 2015 18:29
@jezdez
Copy link
Contributor

jezdez commented Feb 4, 2015

@openjck I'll look tomorrow

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch from cb48cc0 to 011a974 Compare February 4, 2015 19:11
@darkwing
Copy link
Contributor

darkwing commented Feb 4, 2015

On "/en-US/docs/new?slug=Tools", I get this error:

File "/home/vagrant/src/templates/base.html", line 206, in block "site_js"
19:37:25 web.1        |     {{ js(script) }}
19:37:25 web.1        |   File "/home/vagrant/src/vendor/src/jingo-minify/jingo_minify/helpers.py", line 134, in js
19:37:25 web.1        |     urls = get_js_urls(bundle, debug)
19:37:25 web.1        |   File "/home/vagrant/src/vendor/src/jingo-minify/jingo_minify/helpers.py", line 67, in get_js_urls
19:37:25 web.1        |     item in settings.MINIFY_BUNDLES['js'][bundle]]
19:37:25 web.1        | KeyError: 'js'

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch from 011a974 to c376eee Compare February 4, 2015 19:56
@openjck
Copy link
Contributor Author

openjck commented Feb 4, 2015

On "/en-US/docs/new?slug=Tools", I get this error:

Nice catch! Fixed.

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch from c376eee to 3fb71ed Compare February 5, 2015 21:17
@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch from 5b4192d to 7efdfbe Compare February 11, 2015 18:14
@openjck
Copy link
Contributor Author

openjck commented Feb 11, 2015

Definitely an improvement worth making to the codebase. It would be a lot easier to do once all JavaScript and CSS has been migrated to Gulp. How do we feel about postponing until then?

@openjck
Copy link
Contributor Author

openjck commented Feb 11, 2015

Seems like there is a release on http://cdnjs.cloudflare.com/ajax/libs/jquery-scrollTo/1.4.14/jquery.scrollTo.js, any reason we're using not that?

We could use cloudfare, but it only hosts versions newer than the version we're using. We should upgrade the plugin eventually, but the scope of this pull request is just to move existing requirements to Bower.

@openjck
Copy link
Contributor Author

openjck commented Feb 11, 2015

Let's use https://raw.githubusercontent.com/briancherne/jquery-hoverIntent/master/jquery.hoverIntent.js here to not depend on a private hoster

Same with this plugin. GitHub only hosts newer versions of the plugin, and the scope at the moment is just to migrate requirements to Bower. Because the requirements are pulled down locally, we'll have other options if the host goes down, including restoring the plugin source from our Git history.

@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch 3 times, most recently from 1d8a092 to d99c9cb Compare February 11, 2015 19:22
@groovecoder
Copy link
Contributor

This is looking good to me. I especially like the cache-bust method to only change the asset names when the underlying assets change.

I was able to rebase pretty easily too.

How can I make my local dev vm serve up the un-bundled, non-minified assets?

jingo-minify does this with the js and css helpers. I.e., {{ js('bundle-name') }} and {{ css('bundle-name') }} automatically check when settings.DEBUG == True and render separate <script> and <link> elements for each asset listed in the bundle. When settings.DEBUG == False they render the single <script> and <link> elements for the bundled, minifed asset.

Maybe some similar helpers on top of the new cache-bust helper here?

for(var bundleName in jsBundles) {
if(jsBundles.hasOwnProperty(bundleName)) {
var bundle = jsBundles[bundleName];
watchJSBundle(bundleName, bundle);
Copy link
Contributor

Choose a reason for hiding this comment

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

This didn't seem to work for me as I expected. Steps to reproduce:

  1. gulp watch
  2. Change analytics.js (I just added an alert("gulp changed me!"); line to it)
  3. Refresh home page

Expected results:
Should see the "gulp changed me!" alert box.

Actual results:
Nothing. 😦

And only this showing on the console:

(env)vagrant@developer-local:~/src$ gulp watch                                 [55/1609]
[21:16:11] Using gulpfile ~/src/gulpfile.js
[21:16:11] Starting 'watch'...
[21:16:11] Finished 'watch' after 83 ms

It wasn't until I did gulp (to run build-javascript) that the changes took effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaining that. How long was the wait between steps 2 and 3? This sounds like bug 1054257.

There are a few ways around that: we could fix bug 1054257, tell developers to run Gulp on the host machine, or tell developers to wait for a few moments after making file changes. The first option could become a rabbit hole, but one of the others might work in the short term. On the plus side, the delay is no worse than the delay in compiling stylesheets, which we've become used to.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran gulp watch and then went and did some emails for about 20 minutes. 😉 Still no action. 😦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, I can't reproduce. Are you sure media/js/analytics.js was edited, and not one of the files also named analytics.js from the Bower package? Was gulp watch run locally or in the VM?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, media/js/analytics. Ran gulp watch in the VM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have NFS disabled by any chance? I hear that under some conditions (I think disabled NFS is one of them) Vagrant doesn't pick up on file changes.

What happens if you run gulp watch on the host?

The directory media/js/libs/owl.carousel is not removed because
jingo-minify still refers to stylesheets from that directory. The
directory can be removed once Gulp is used to build CSS.
The directory media/js/libs/prism is not removed because jingo-minify
still refers to stylesheets from that directory. The directory can be
removed once Gulp is used to build CSS.
@openjck openjck force-pushed the bug-1119397-initial-gulp-and-bower branch from d99c9cb to 06b1291 Compare February 11, 2015 21:48
@openjck
Copy link
Contributor Author

openjck commented Feb 11, 2015

How can I make my local dev vm serve up the un-bundled, non-minified assets?

Good question. I never found unbundled assets to be especially helpful, but I can see a case for them if someone really needs to use the debugger with the original symbols. This will take some thought. It will need to be something more expansive than changing cache_bust, since cache_bust takes the filename of a minified resource as input.

@stephaniehobson
Copy link
Contributor

I find unbundled CSS to be quite helpful and would be sad it it went away.

@groovecoder
Copy link
Contributor

My McDonald's idea:

  1. Create new js(bundle_name) and css(bundle_name) helpers that replace the jingo-minify helpers
  2. Each helper:
    1. if settings.DEBUG == True: render a <script> or <link> element for each file in the bundle
    2. if settings.DEBUG == False: render a single <script> or <link> element for the combined, minified file using cache_bust to create the filename.

Seem logical?

@jezdez
Copy link
Contributor

jezdez commented Feb 12, 2015

@groovecoder -1 on writing our own Python stuff to handle bundles, we should follow the best practices of the community and use django-pipeline. It's what bedrock has been moved to, too.

@groovecoder
Copy link
Contributor

👍 using best practices python.

That would mean moving the bundle declarations out of gulpfile.js and back into settings.py.

@openjck
Copy link
Contributor Author

openjck commented Feb 12, 2015

django-pipeline also does concatenation and minification. Is it worth the effort to get django-pipeline and Gulp to talk to each other if django-pipeline does the same job anyway?

@groovecoder
Copy link
Contributor

That's a big question. Let's start a thread with dev-mdn and dev-webdev about it?

@openjck
Copy link
Contributor Author

openjck commented Feb 12, 2015

It would be easiest to do this either entirely with a Django plugin or entirely with Gulp. If we do this entirely with Gulp, and we need uncompressed assets locally, we'll need to run Gulp at deploy time to treat different environments differently.

@jezdez
Copy link
Contributor

jezdez commented Feb 12, 2015

@openjck Agreed, I don't think we should mix gulp and another tool that does asset compilation, concatenation, minification, etc – that was my point earlier really. I'm happy to use gulp, but we should use it completely and not compromise. Looking back at https://lincolnloop.com/blog/integrating-front-end-tools-your-django-project/ this is what they went with, staticfiles only for the management of Python side static files and gulp for any processing.

@openjck
Copy link
Contributor Author

openjck commented Feb 12, 2015

@openjck openjck closed this Feb 12, 2015
@openjck openjck deleted the bug-1119397-initial-gulp-and-bower branch February 12, 2015 20:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants