The .then and .catch methods in runtime/OutMixins.js are broken

[Hesulan]

I finally got around to looking at the newly added .then method from #423 , and it turns out that it's nowhere near spec-compliant: attempting to call .catch causes an error to be thrown ("TypeError: fn is not a function", referring to the undefined passed as the first argument to .then(fn, fnErr)), the handlers are called synchronously, and .then itself ceases to function properly once the stream is finished (subsequent calls return a Promise resolved with undefined).

There are only 3 possible solutions I can think of:
1. Let .then be non-compliant. It will only work if called before the stream is finished, among other things.
2. Attach a Promise instance to the stream regardless of whether .then is ever called or not. The renderer would then always require a native or polyfilled Promise implementation, even if .then is never actually used.
3. Write a partial Promise implementation directly into the renderer. I could do this if needed, but it would probably be better to simply require a native or polyfilled implementation.

EDIT: It seems I was mistaken about it ceasing to function after 'finish' is called, though it doesn't quite appear to function correctly in the first place.

EDIT 2: I'm actually getting all sorts of weird problems from this code and I'm not entirely sure what's causing most of them. In case someone else wants to take a stab at it, here's a spec-compliant solution, but it caused the test suite to throw an out-of-memory error when I tried it:

then: function(fn, fnErr) {
    var self = this;
    return new Promise(function(resolve, reject) {
        self.on('error', reject);
        self.on('finish', resolve);
    }).then(fn, fnErr);
}

[Hesulan]

So I just now realized why that was throwing an error: The stream itself is being passed to 'finish', which is then passed to resolve, which attempts to call the stream's .then method, which leads to an infinite loop. It needs to instead pass the result string to resolve, but the test suite currently depends on the resolved value being the stream itself (which definitely violates the Promises/A+ spec, since the stream is a thenable).

In other words, the tests need to be rewritten. Then the above solution, with self.on('finish', function(data){resolve(data.getOutput());}); instead of self.on('finish', resolve);, should work perfectly. (I'm pretty sure it's data.getOutput(), but double check that...)

I won't get the chance to do that tonight, or probably tomorrow either, so if anyone else would like to do the honors, be my guest. If not, I'll hopefully get to it this week.

[mlrawlings]

Hmm... I see the issue... Unfortunately, our goal with #389 was to pass the stream as the value so you could do something like:

template.render().then(out => out.appendTo(document.body));

[mlrawlings]

@patrick-steele-idem Maybe we need to consider having a RenderResult that is separate from the stream.

var result = template.renderSync();
var out = template.render();

out.on('finish', function(result) {});
out.then(function(result) {});

template.render({}, function(err, result) {});

Pretty much everything in the outMixins except then/catch would be a method of the RenderResult.

result.out could point to the original stream if necessary.

[Hesulan]

If the point is to get the stream itself when it's done rendering, then doesn't ".on('finish')" already pretty much do that?

Both in theory and practice, the point of Promises has always been to handle a single, immutable result (or error) the same way regardless of whether it is known yet or not. In this case, I feel that would be the resulting string.

[Hesulan]

In other words, it seems to me that ".render().then()" should behave like ".renderSync()", but allowing for asynchronous values within the template.

[Hesulan]

Either way, for a thenable to be passed to the first callback given to ".then()" explicitly defies the core of what a javascript "Promise" is. A separate non-thenable version of the stream like what you described would be the only acceptable way to do that.

[Hesulan]

Hm. Somehow didn't notice that ".renderSync()" now returns a stream, too. In that case, this either needs to pass around a non-thenable version of the stream, or not be named ".then".

[patrick-steele-idem]

Maybe we need to consider having a RenderResult that is separate from the stream.

That's probably the right thing to do.

What are your thoughts on adding a try...catch to wrap the rendering of the template so that we can emit an error event in the catch clause? We would only do this if an out is not provided to the template.render() method.

[Hesulan]

What if we were to simply rename .then? The only real problem here (aside from the broken .catch method) is that we're passing an object (the stream) with a .then method to the .then() callback, which explicitly violates the specification. But if it were named something else, like .promisify(), .and(), or literally anything other than .then() or .catch(), then my example from "Edit 2" in the original post would work perfectly as-is.

[Hesulan]

Fixed in #443. Thanks!