Magento and Inline JavaScript (Part 1)

I want to see zero inline JavaScript in #magento2 because modern development.

And here’s the real security reason: http://www.html5rocks.com/en/tutorials/security/content-security-policy/…
Brendan Falkowski, on Twitter

I too want to see zero inline JavaScript in Magento 2, but my reasons are not primarily security related. I'd also like to see zero inline JavaScript in Magento 1, but I don't really have hopes of implementing CSP in our Magento 1 deployments. Did I mention I'd also like to be king of all Londinium and wear a shiny hat?

What are the problems with inline JavaScript?

Inline JavaScript is very difficult to maintain, because it is difficult to debug, and it is difficult to find.

Placing breakpoints in inline JavaScript usually devolves to me manually inserting debugger; statements into the code, because browsers make it so difficult to find inline JavaScript. Perhaps I forget to remove one of these, which is always tremendous fun.

Of course, finding inline JavaScript is a challenge in itself, because the compiled HTML is not the same as all of the templates that went into it. I often find myself grepping my code base for a certain bit of JavaScript that I hope is unique enough to identify it.

And let's not forget the security concerns that @falkowski points out. It is not possible to implement Content Security Policy in Magento 1, even if we kept our code free of inline script tags, because Magento core has inline script tags all over the place. I think it would be nice not to have to worry about this as we port our code to Magento 2. We'll have enough to worry about, and I for one don't want to be the one responsible for us not being able to roll out CSP when Magento no longer makes that impossible.

Why do we use it anyway?

It seems likely to me that we would depend on inline script tags far less if the Magento core code did not use them so heavily. However, it does. And it does because Magento 1 is old, and refactoring this would be an inordinate amount of work, even more so when we remember that every button which uses the onclick attribute to implement behaviour is also inline code.

But let's not forget that inline script tags are really useful. That's why we are addicted. I have yet to see a faster way of transporting information from the server to the client, or a faster way of adding dynamic code. This convenience is seductive, yes, but it costs us in the long run. Over a few posts, I will give a few examples, and show what I believe to be a better, more sustainable way of approaching the problem. I've tried to select them so that each shows a legitimate use of inline script tags, and why it seems like a good idea, and why maybe it isn't.

It's Just a Tiny One Line Snippet

Often, it seems better to add something small to the HTML to initialise some jQuery plugin or suchlike, because abstracting it into its own file seems like a waste of time. In my experience, this is sometimes the case, but usually it ends up taking more time in the long run, and doing a quick job the first time disinclines us to do some things that we should be doing.

Suppose I wish to include a carousel on a category page. I have the markup, and I am using Owl Carousel for this. I have the html set up correctly, and I have the jQuery and the Owl plug in loaded. Now what?

The solution, of course, is:


<div id="my-amazing-slider">
  ...
</div>
<script>
$j('#my-amazing-slider').owlCarousel();
</script>

Now imagine one of the many settings needs to be changed. I see it in the outputted HTML, but I'm not sure where to edit this. Maybe if I grep for "my-amazing-slider", it should help me to find it. After all, that is an id, which should be unique. But I find it nowhere. So I have to go and bother the person who originally developed this functionality, and they tell me that it can be found in the CMS. So I dutifully apply this change locally, test it. Then apply it to dev, show my manager. Then apply it to UAT, so my manager can show the client, and then at some point I have to push this change to live.

That sure is a lot of work for a three line diff. All because nobody took the time in the first instance to do the job properly. Let's not forget that I also just turned down an opportunity to make it right, which was a mistake as well. So now our code looks like this:


<div id="my-amazing-slider">
  ...
</div>
<script>
$j('#my-amazing-slider').owlCarousel({
  "addClassActive": true
});
</script>

I think we can do better, and I think we should do better. So lets remove that inline JavaScript, and replace it with a comment:


<div id="my-amazing-slider">
  <!-- Slider configured in category-slider.js -->
  ...
</div>

And then, clearly, we need to write category-slider.js:


;(function configureCategoryPageSlider () {
  'use strict';

  var config = {
    "addClassActive": true
  }
  
  var sliderId = "my-amazing-slider"
  var slider = $j('#' + categorySliderId)

  function run () {
    slider.owlCarousel(config)
  }
  
  $j(run)
})();

Now I realise this is a little bit of a straw man. I cheated on the find-ability scale somewhat by adding a comment. But comments are okay when they are helpful. That's why we have comments. We could potentially save someone having to ask how the slider works, or at least save them some time. If you wanted to make inline scripts easier to find, there is nothing stopping you adding a comment at the start and end of every CMS page, every CMS block, and every template, and then everything would be easy to find. I'll leave the decision of whether or not you would like to do that up to you.

We also enabled strict mode, which is a good thing. We can't enable strict mode globally in Magento 1, although it's fun to do so sometimes. But we can use it within our functions, to make sure we're not doing anything nasty. It's not all roses in this change, though. There are some thorns.

I have introduced a new problem, one that I conveniently avoided due to decreasing the line length of my code samples: what do you want to call this file? Perhaps you have conventions in place for the unambiguous naming of things, but more likely, you have to choose something to call it. Naming things is one of the hard problems. I don't think that skin/frontend/package/theme/js/category-slider.js is a bad place to start, though.

This example avoids the difficulty of how to include this JavaScript file, as well. We get a convenient layout handle for each category. But you do not get layout handles for every static block that is used. Were this slider inside a static block, we might be causing ourselves pain.

There is also the concern that we are adding a new file, and therefore a new HTTP request. Especially when you consider that Magento's script combining often doesn't work, and is of questionable value, even when it does.

Finally, let's not forget that I just turned what was a twenty minute bug fix (because I have to repeat it four times) into an hour or so and a commit, pull request, and deployment.

I don't mean to be dismissive. These are all real concerns. I'm going to argue that these are all concerns worth overcoming, though.

Say we want one of these on the home page, and one on certain configurable product pages, and one on the contact page, and we're going to feature some people on a creators page, and we have a blog, which obviously has to have a carousel, although the blog carousel is going to be a little bit different. And because this creeps over the course of the project, we don't even realise that we duplicated the knowledge of how carousels are supposed to look and be initialised across our entire site. Now imagine that we desperately need to add a style to the active slide. We could go and find find all of the places we are using a carousel, and just hope that we got them all, then handle the embarrassment and support cost as we fix up the last few straggling carousels that we missed.

Or, we could change our script. Perhaps rename it to carousel.js, and then:


;(function configureCategoryPageSlider () {
  'use strict';

  var config = {
    "addClassActive": true
  }
  
  var sliderSelector = ".owl-carousel"
  var sliders = $j(sliderSelector)

  function run () {
    sliders.owlCarousel(config)
  }
  
  $j(run)
})();

I don't mean to brag, but that was a very easy change, and we just implemented sliders across our entire site. Of course you do have to change your XML layout, and move it under the default handle. And change the HTML a little bit where we use the carousel. At this point, we're probably saving ourselves time over the inline script approach.

But earlier, I stipulated that the blog carousel was going to be a little bit different. Now we have a problem, because our carousels are not all the same. A terrible flaw, which wouldn't be an issue if we'd kept it the old way. Well, we can use custom attributes. It's what Zurb do with their Foundation framework, and they come in handy.


;(function configureCategoryPageSlider () {
  'use strict';

  var defaultConfig = {
    "addClassActive": true
  }
  
  var owlConfigPrefix = 'owl'
  var sliderSelector = '.owl-carousel'
  
  var sliders = $j(sliderSelector)

  function run () {
    sliders.each(initSlider)
  }
  
  function initSlider (slider) {
    var $slider = $j(slider)
    
    $slider.owlCarousel(getTotalConfig($slider))
  }

  function getTotalConfig ($slider) {
    return $j.extend(
      {},
      getCustomConfig($slider),
      defaultConfig
    )
  }

  function getCustomConfig ($slider) {
    var allData = $slider.data()
    var owlConfigData = _.filter(allData, isOwlConfig)
    var customConfig = {}
    
    _.each(owlConfigData, addToConfig, customConfig)
    
    return customConfig
  }
  
  function isOwlConfig (value, key) {
  	return key.indexOf(owlConfigPrefix + '-') === 0
  }
  
  function addToConfig (value, key) {
    var configKey = convertAttributeKeyToConfigKey(key)
    
    this[configKey] = value
  }

  /**
   * Strip "owl-" and camel case
   *
   * @example
   * convertAttributeKeyToConfigKey('owl-auto-play')
   * -> 'autoPlay'
   */
  function convertAttributeKeyToConfigKey (key) {
    var tokens = key.split('-')
    
    return tokens[1] + _.map(tokens.slice(2), firstToUpper).join('')
  }
  
  function firstToUpper (word) {
  	return word[0].toUpperCase() + word.slice(1)
  }
  
  $j(run)
})();

So we ended up writing some code, and our complexity has increased, and we can definitely question the value of this. But we can now have all of our sliders "Just Work", and if some need to be different, that's easy too:


<div
  class="slider"
  data-owl-auto-play="true"
  data-owl-stop-on-hover="true"
>
  ...
</div>
<script>

The code isn't all that complicated, either. All of the stuff that deals with the data API, and converting things to camel case can and should be abstracted into some other library, for when we need to add tabs in a similar fashion.

At this point, I'd say we're in a place where we have a framework in place to pretty much satisfy anything a client or boss can throw at us. Maybe add it to the company default theme, with a handy PR, then a bit in the README, and it looks like we did a good job.

I know it might seem that this example is contrived, and I have needlessly over-engineered this framework. I think that's hogwash. We've built a sane API, that's totally declarative and totally transparent, and it only took 50 lines or so. Less if we consider that some of the functions I declare should be library methods instead of defined in place. But remember, I'm not advocating for us to write all JavaScript like the final code sample. That's impossible, and even if it wasn't, it's overkill. However, it is vital not to rule this out with our initial decisions. I don't think that the final example would have evolved if we hadn't factored out category-slider.js in the first instance. We'd have a mess of inline scripts on our hands now.