Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{?exists} and {^exists} now supports values from a promise #753

Merged
Merged
96 changes: 71 additions & 25 deletions lib/dust.js
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,11 @@
return this;
};

Copy link
Contributor Author

@samuelms1 samuelms1 Nov 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these docs straight from http://www.dustjs.com/docs/helper-api/. Looks like other docs are missing from some chunk methods that are on the helper-api page. I could add these if they aren't considered bloat. This Chunk#map method is great and the docs really made it's usage clear to me. As I was looking at making the code both smaller and resuable, it seemed apparent that what I really needed was a new map method for thenables, so I created the mapThenable(...) method. I'd like to put it on the Chunk object, but that would be altering the public API of that and I'm not sure that is desirable. I'm also wondering if I should move the Chunk#renderError method off of the Chunk prototype for the same reason or if it's simple, generic, and useful enough to keep.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have the right separation. I think mapThenable is not something that a general public API would necessarily need, but renderError is nice. We can add it to the docs.

/**
* Inserts a new chunk that can be used to asynchronously render or write to it
* @param callback {Function} The function that will be called with the new chunk
* @returns {Chunk} A copy of this chunk instance in order to further chain function calls on the chunk
*/
Chunk.prototype.map = function(callback) {
var cursor = new Chunk(this.root, this.next, this.taps),
branch = new Chunk(this.root, cursor, this.taps);
Expand All @@ -740,6 +745,35 @@
return cursor;
};

/**
* Like Chunk#map but additionally resolves a thenable. If the thenable succeeds the callback is invoked with
* a new chunk that can be used to asynchronously render or write to it, otherwise if the thenable is rejected
* then the error body is rendered if available, an error is logged, and the callback is never invoked.
* @param {Chunk} The current chunk to insert a new chunk
* @param thenable {Thenable} the target thenable to await
* @param context {Context} context to use to render the deferred chunk
* @param bodies {Object} may optionally contain an "error" for when the thenable is rejected
* @param callback {Function} The function that will be called with the new chunk
* @returns {Chunk} A copy of this chunk instance in order to further chain function calls on the chunk
*/
function mapThenable(chunk, thenable, context, bodies, callback) {
return chunk.map(function(asyncChunk) {
thenable.then(function(data) {
try {
callback(asyncChunk, data);
} catch (err) {
// handle errors the same way Chunk#map would. This logic is only here since the thenable defers
// logic such that the try / catch in Chunk#map would not capture it.
dust.log(err, ERROR);
asyncChunk.setError(err);
}
}, function(err) {
dust.log('Unhandled promise rejection in `' + context.getTemplateName() + '`', INFO);
asyncChunk.renderError(err, context, bodies).end();
});
});
}

Chunk.prototype.tap = function(tap) {
var taps = this.taps;

Expand Down Expand Up @@ -861,6 +895,12 @@
var body = bodies.block,
skip = bodies['else'];

if (dust.isThenable(elem)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you turn this into a little reusable func that we can use for exists and notexists both? Look above at getWithResolvedData as an example-- just a little helper func not attached to the Chunk prototype.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by "this" I meant the code inside the if() block that is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was definitely redundant between the two methods. I made a mapThenable method to address it.

return mapThenable(this, elem, context, bodies, function(chunk, data) {
chunk.exists(data, context, bodies).end();
});
}

if (!dust.isEmpty(elem)) {
if (body) {
return body(this, context);
Expand All @@ -876,6 +916,12 @@
var body = bodies.block,
skip = bodies['else'];

if (dust.isThenable(elem)) {
return mapThenable(this, elem, context, bodies, function(chunk, data) {
chunk.notexists(data, context, bodies).end();
});
}

if (dust.isEmpty(elem)) {
if (body) {
return body(this, context);
Expand Down Expand Up @@ -970,27 +1016,31 @@
* @return {Chunk}
*/
Chunk.prototype.await = function(thenable, context, bodies, auto, filters) {
return this.map(function(chunk) {
thenable.then(function(data) {
if (bodies) {
chunk = chunk.section(data, context, bodies);
} else {
// Actually a reference. Self-closing sections don't render
chunk = chunk.reference(data, context, auto, filters);
}
chunk.end();
}, function(err) {
var errorBody = bodies && bodies.error;
if(errorBody) {
chunk.render(errorBody, context.push(err)).end();
} else {
dust.log('Unhandled promise rejection in `' + context.getTemplateName() + '`', INFO);
chunk.end();
}
});
return mapThenable(this, thenable, context, bodies, function(chunk, data) {
if (bodies) {
chunk.section(data, context, bodies).end();
} else {
// Actually a reference. Self-closing sections don't render
chunk.reference(data, context, auto, filters).end();
}
});
};

/**
* Render an error body if available
* @param err {Error} error that occurred
* @param context {Context} context to use to render the error
* @param bodies {Object} may optionally contain an "error" which will be rendered
* @return {Chunk}
*/
Chunk.prototype.renderError = function(err, context, bodies) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good reusable func

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Should I move it off of the Chunk object? When I added it originally I wasn't considering that I was updating a public api that's pretty well documented here: http://www.dustjs.com/docs/helper-api/

var errorBody = bodies && bodies.error;
if (errorBody) {
return this.render(errorBody, context.push(err));
}
return this;
};

/**
* Reserve a chunk to be evaluated with the contents of a streamable.
* Currently an error event will bomb out the stream. Once an error
Expand All @@ -1002,8 +1052,7 @@
* @return {Chunk}
*/
Chunk.prototype.stream = function(stream, context, bodies, auto, filters) {
var body = bodies && bodies.block,
errorBody = bodies && bodies.error;
var body = bodies && bodies.block;
return this.map(function(chunk) {
var ended = false;
stream
Expand All @@ -1025,11 +1074,8 @@
if(ended) {
return;
}
if(errorBody) {
chunk.render(errorBody, context.push(err));
} else {
dust.log('Unhandled stream error in `' + context.getTemplateName() + '`', INFO);
}
chunk.renderError(err, context, bodies);
dust.log('Unhandled stream error in `' + context.getTemplateName() + '`', INFO);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a logic difference. Previously dust.log(...) was only called when there was no render body whereas now it is always logged. I think it's ok.

if(!ended) {
ended = true;
chunk.end();
Expand Down
6 changes: 3 additions & 3 deletions test/templates.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function render(test, dust) {
expect(messageInLog(dust.logQueue, test.log)).toEqual(true);
}
if (typeof test.expected !== 'undefined') {
expect(test.expected).toEqual(output);
expect(output).toEqual(test.expected);
}
done();
};
Expand Down Expand Up @@ -124,7 +124,7 @@ function stream(test, dust) {
expect(messageInLog(dust.logQueue, test.log)).toEqual(true);
}
if (typeof test.expected !== 'undefined') {
expect(test.expected).toEqual(result.output);
expect(result.output).toEqual(test.expected);
}
done();
};
Expand Down Expand Up @@ -196,7 +196,7 @@ function pipe(test, dust) {
expect(messageInLog(dust.logQueue, test.log)).toEqual(true);
}
if (typeof test.expected !== 'undefined') {
expect(test.expected).toEqual(result.data);
expect(result.data).toEqual(test.expected);
}
if(calls === 2) {
done();
Expand Down
19 changes: 19 additions & 0 deletions test/templates/all.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,18 @@ return [
message: "should setup base template for next test. hi should not be part of base block name"

},
{
name: "{?exists} supports promises and uses correct context",
source: "{#a}{?b}{test}{/b}{/a}",
context: {
a: {
b: FalsePromise(null, { test: "BAD" }),
test: "GOOD"
}
},
expected: "GOOD",
message: "{?exists} supports promises and uses correct context",
},
{
name: "issue322 use base template picks up prefix chunk data",
source: '{>issue322 name="abc"/}' +
Expand Down Expand Up @@ -584,6 +596,13 @@ return [
expected: "false",
message: "empty array is treated as empty in exists"
},
{
name: "empty array resolved from a Promise is treated as empty in exists",
source: "{?emptyArrayFromPromise}true{:else}false{/emptyArrayFromPromise}",
context: {"emptyArrayFromPromise": FalsePromise(null, [])},
expected: "false",
message: "empty array resolved from a Promise is treated as empty in exists"
},
{
name: "empty {} is treated as non empty in exists",
source: "{?object}true{:else}false{/object}",
Expand Down