Background

All JavaScript files in our codebase are delived to the browser by ResourceLoader. In production, these are passed through JavaScriptMinifier, which parses every token in every file, to determine whether it can be compressed (by removing whitespace). It does this at fast and with additional caching on top. Periodically (every 1000 characters or so) we try to break up the line.

When a JavaScript file (in a gadget, or extension) contains unknown or invalid syntax, we accept that with such invalid input, the minifier may produce invalid output (GIGO principle). After all, we don't need to support ES 16.0 code in our minifier, when our browsers require ES 7.0 code, because we wouldn't be writing ES 16.0 code in the first place, as we couldn't ship it anyway. We'll support it when we need it. So far so good.

As the JavaScript specification is under continuous development with new specifications each year, it is inevitable that some unknown syntax will be attempted to be used in a gadget or extension. Equally inevitably, is that developers may occasionally forget to run a linter, or syntax checker, or misconfigure the linter in one of their source repositories. Or in the case of gadgets, we currently still have a (temporary) option that allows a gadget to skip this syntax invalidation step (T362855).

1. The most likely and least problematic failure scenario is under-compression: Since the unknown syntax isn't supported yet, we could compress better and remove more spaces/comments in the future for greater effiency. This is harmless.

2. The least likely but unavoidable failure scenario is over-compression: If the future/unknown syntax involved a token that is similar to an existing syntax where a space is optional, but it requires the space, there's a small chance we may be compressing it too much. This is unavoidable, but very uncommon. The good thing is that this will be deterministic and immediately cause a syntax error or other behaviour change that should be easy to spot and reliably introduce. In any event, remember that we're talking about cases where a mistake has already happened: we have skipped/misconfigured a linter and unsupported code has made it into production, and now the minifier is running over it.

3. There is a third scenario: This is fairly common, and I believe entirely avoidable. To break up the payload we try to spread it over multiple lines roughly every 1000 characters. Today, we do this by, after 1000 characters (or shortly before it) insert a new line if the syntax indicates that it is safe to do so. In practice this happens after every 990-1050 characters or so. The problem with this is that, for syntax we don't know that is masqerading as something we do know, our information about what is "safe" is just as likely to be right as it is to be wrong. This means we may be production invalid output for something that was actually valid on input (but unknown/newer than we support).

In these cases, it's "fine" for us to fail. We can't guruantee perfectly compressing something that we don't recognise or support yet. That's the interface contract. The question is, how do we fail?

Problem

Today, we fail in quite a subtle way. The inserted line break will be dynamically inserted at run-time in the middle of a compressed JavaScript blob, a blob that is automatically generated and then transmitted between server and browser. It is not for humans to review. It will be syntactically different or syntactically invalid in some subtle way that you're not likely to notice.

Unit tests? 100% code coverage is infeasible and expensive. If you did cover the affected code with QUnit and run it via MediaWiki/Quibble in WMF CI, you may potentially catch this. But this is unlikely. While issue 1 and 2 above are deterministic, issue 3 (which this task is about) is not a deterministic. It only affects 1 in every 1000 characters. Depending on what else the payload contains, it may only happen sometimes, and vary on the page, or what you have in cache etc. Unless the syntax and the offset line up exactly, you won't be able to reproduce it!

Solution

Today we remove all unneeded white space and line breaks, and every 1000th character or so we also try to insert a line break of our own. (If we can't, we'll do it at the next safe oppertunity). Given that a line in the source code can contain many different tokens (e.g. "var foo = fn(a + b);" is 10 tokens), our inserted line breaks are usually at positions where the original code did not have any, and the ones that were there, we remove for optimal compression. Again, this is fine when you have a correctly configured linter, and is fine when the input contains only supported syntax.

I suggest we instead, when we would like to insert a line break, to preserve the next line break that was already in the input, never create our own.

Imagine a compression break preference of 20 characters (production: 1000).

// My documentation comment
var foo = x(a + b) / y();
foo++;

// Start
var bar = y(
  z(a),
  z(b),
  z(c),
  z(d),
  z(e)
);

var foo=x(a+b)/y();
foo++;var bar=y(z(a)
,z(b),z(c),z(d),z(e));

var foo=x(a+b)/y();foo++;
var bar=y(z(a),z(b),z(c),
z(d),z(e));

Outcome

1. A compressor should never output invalid code when the input was valid. This gets us closer to that ideal. If you're using invalid syntax, you should find out from you linter (locally during dev, or in CI before merge) or from a runtime error from the browser informing you about it. You don't want the minifier subtly breaking or changing behavior of the code in a non-deterministic way.
2. Compressed payloads will internally build slightly longer lines than before. Previously we inserted a line break every 990-1010 chars (either before or after the end of the "next" token when we exceed the threshold, given token lengths of ~10 chars). This will now be every 1000-1080 chars (at the end of an original line, given 50-80 char lines). This has no impact on performance or debuggability.
3. Input to ResourceLoader that is already compressed will now have the same line length as the original, instead of gaining additional line breaks. This has no impact on performance or debuggability. This applies to gadgets or core/extension modules that embed an upstream JavaScript dependency, and copy an already-minified version of that dependency.