Allow base scripts to run within a module loader

[colinrotherham]

Hi,

So I can avoid multiple <script> includes, I'm adding base modules via Browserify (same principle for Webpack, Rollup etc) using this sort of syntax:

require('govuk/selection-buttons');
require('govuk/analytics/analytics');

Problem is, I've noticed a lot of the modules use this but that wrongly assumes this == window.

To resolve, this fork ensures all base modules attach to the window global for compatibility. I've also noticed use of root internally so I've further rolled this out to all modules for consistency.

These changes make no assumption that the GOVUK global variable exists. Some modules tested for this, some didn’t. Checks for this have now been tidied up.

This is a stepping-stone to a nice dependency-injected asynchonrous module future, without adding/removing properties on the window.GOVUK global.

Hope this helps.

[tvararu]

I'm fine with this and it's something I've thought about before, good work!

👍 from me.

[colinrotherham]

Thanks @tvararu,

A further (future) approach could be to:

1. Convert each *.js base script to CommonJS module syntax
2. Add module.exports return values (or similar shim like UMD)
3. Keep the window.GOVUK.moduleName mechanism for backwards-compatibility

Perhaps I've got the ball rolling.

[NickColley]

Hey @colinrotherham 👋

Thanks a lot for this PR 💃

I'm 👍 have a thought...

To reduce the amount of time you have to refer to root we could have it scoped inside the closure then reassign back to the window at the end?

(function (global) {
  var GOVUK = global.GOVUK || {};

  GOVUK.module = function () {};

  global.GOVUK = GOVUK;
})(window);

We also need to update https://github.com/alphagov/styleguides/blob/master/js.md#modules if this lands.

[colinrotherham]

Thanks @NickColley.

Yeah good idea, I've re-pushed my commit. I stuck with root as it was already a convention so didn't want to introduce anything new—now it’s global instead, as suggested.

This way, the minifier can reduce the closure’s var GOVUK to var a or similar, saving a few more bytes in each base script. But, we could go further if needed, see below:

This suggestion

GOVUK.performance = GOVUK.performance || {};

GOVUK.performance.method1 = function() {
  /* Do something */
};

GOVUK.performance.method2 = function() {
  /* Do something else */
};

Future improvement

function method1() {
  /* Do something */
};

function method2() {
  /* Do something else */
};

GOVUK.performance = {
  method1: method1,
  method1: method2
}

Which when minified reduces down nicely:

function b() {
  /* Do something */
};

function c() {
  /* Do something else */
};

a.performance = {
  method1: b,
  method1: c
}

We could do the same and remove Instance.prototype used throughout the other base scripts as it's not very minifier-friendly and makes quite a wordy output*.

This can be a starting point though, perhaps move the other suggestions to a couple of new issues?

*Unless you’re actually making thousands of instances (e.g. a canvas particle generator) and need the prototype tree to reduce memory usage, it’s not worth it.

[colinrotherham]

Sorry, I've got "trim whitespace" enabled in my editor.

[tvararu]

Trim away. ✂️

[dsingleton]

👏 to moving towards more formal modules - i've been looking at something like UMD as part of de-rails-ing some of the existing JS dependencies, so this is a great step.

This can be a starting point though, perhaps move the other suggestions to a couple of new issues?

👍

[robinwhittleton]

A possible follow-up PR: we explicitly bind $ to global.jQuery in the core JS (e.g. stop-scrolling-at-footer.js) but not in the analytics files (e.g. download-link-tracker.js). This probably needs adding.