Monday, April 30, 2018

Debugging: Deja Vu

Debugging: Deja Vu

I’m going to share some stories on debugging with you, because I’m proud of them. After writing up the first story, I’m no longer particularly proud, but I still want to share the story. Here’s the first: a bug that seemed quite familiar.

After some trouble with mocking API requests I decided that supporting mocking in the browser isn’t as important as supporting mocking at all, so I installed mock-require (I didn’t get proxyquire to work). Now, to confirm that the test bundle script actually omitted the API mocking code from the bundle, I loaded the test suite in the browser. Guess what? Errors everywhere! Or rather, error everywhere. Every test case gave the same error:

TypeError: Cannot assign to read only property 'length' of function 'function () {
          old.apply(self, arguments);
    at new Assertion (/build/test.citation.js:1810:30)
    at new Assertion (/build/test.citation.js:1799:25)
    at new Assertion (/build/test.citation.js:1799:25)
    at new Assertion (/build/test.citation.js:1799:25)
    at expect (/build/test.citation.js:1775:12)
    at Context._callee2$ (/build/test.citation.js:3582:35)
    at tryCatch (/build/citation.js:7891:17)
    at Generator.invoke [as _invoke] (/build/citation.js:8064:22)
    at Generator.prototype.(anonymous function) [as next] (/build/citation.js:7934:21)
    at step (/build/test.citation.js:3542:221)

Naturally, like a good programmer, I immediately googled the error message in conjunction with the various tools and frameworks I used for this bundle, instead of looking at the stack trace showing that something’s wrong with expect.js. Anyway, after some time, I found a GitHub issue describing exactly the problem I was having. Scrolling through the responses, I was stunned:

larsgw commented on Jul 29, 2017
+1: Same issue, but for all assertions

Somehow, I reported the same issue 10 months ago, but the site, running a bundle from 2 months old didn’t have the problem. Apparently, I had fixed the issue earlier, but forgotten about the solution, and I couldn’t figure out what that solution was. So I started debugging. First of all, the offending code wasn’t different from any other working bundle. It registered a bunch of functions as properties to a function, and it choked on the length property. Makes sense, the length property isn’t writable on functions. But running some simple test code showed this shouldn’t be the problem:

let f = function () {}
console.log(f.length) // 0
f.length = 3   // (no error)
console.log(f.length) // 0

Sure, it didn’t actually do anything, but it didn’t throw an exception either. Besides, this was the exact same lines of code as in the GitHub repo, so how could that be the issue? The only thing I could do was comparing with the working examples. Diffing my bundle against the published one, which was difficult, because it’s generated code. Checking out commits until I found out which one worked, which was a pain because I repeatedly forgot to reinstall dependencies, something that could be critical.

Sidebar (minor spoilers): the previous time I ‘solved’ the issue, it was much easier, because it was the first time setting the system up, so instead of referring to older commits as working examples, I looked at the docs.

After a long while I realised what the workaround was earlier: instead of bundling expect.js with Browserify, I included it as a script (as recommended) and created a wrapper that exposed the expect.js module and simply grabbed and exported the global expect variable exposed by the script. I thought this was because the order of requiring the scripts mattered, but some testing proved this wasn’t the case. No, actually including expect.js into a Browserify bundle with Babelify transform on, or even simply running it through the Babel compiler caused the error.

Back to diffing bundles I guess: what are the differences between a babel-ed file and its source code if there isn’t really any syntax that needs to be transformed, or APIs that need to be polyfilled?

Turns out, not much. Between those files, the only real difference was the location (or with comments: false the existence) of comments, and some style differences caused by Babel’s code generator.

And 'use strict'.

Apparently, 'use strict' makes assignments that otherwise fail silently throw an error. If I had read that documentation earlier, or if I had payed proper attention to the Function.prototype.length docs (linked above), I would have known. Now it’s just a boring ending to a long journey. But hey, at least I learned some stuff.

Solving this issue requires either a big change in the internals of a toolkit that hasn’t had an update in 4(!) years, or a workaround. I don’t want to use the workaround of including two extra files anymore, now that I know what is causing the issue, but the other workaround proves to be a problem itself, involving outdated documentation and a bug in Babelify. More on that later.

On a related note: I’m working on a new release for Citation.js, improving the parsing plugin system. The API should be pretty stable now, apart from the namespaces being prone to change, so I might change the schedule to one update with all current API changes instead.