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

samuelms1 · 2016-11-07T19:30:52Z

I logged this as bug #752 - {?exists/} behavior different when using promises.

sethkinast · 2016-11-07T20:11:35Z

I think this won't quite be right because when you use await in this way, you'll be shifting the context of the body. This is very close, though.

I think something like this might fail with your change:

{
  a: {
    b: {
      test: Promise.resolve("BAD")
    },
    test: Promise.resolve("GOOD")
  }
}

{#a}{?b}{test}{/b}{/a}

For the test runner failure, it's because Node 0.10 doesn't have Promise-- use the "FalsePromise" constructor at the top of the test file.

sethkinast · 2016-11-07T20:15:17Z

You probably want to use chunk.map similar to how Chunk#await does it, but you need to only check the truthiness of the body rather than delegating to Chunk#section, which is what await will do.

It hits this code:

if (bodies) {
  chunk = chunk.section(data, context, bodies);
}

samuelms1 · 2016-11-08T16:59:16Z

@sethkinast thanks for the comments! I got pretty busy yesterday, so I had to switch over to other work. I'm going to take another look.

sethkinast · 2016-11-08T17:56:32Z

Yep no rush! I appreciate you taking the time to work on a patch :)

…ed {expected} to equal {actual}" rather than vice versa

…errors. Previous implementation had an issue where the context was updated to the resolved promise data which was not intended as sethkinast pointed out.

samuelms1 · 2016-11-08T18:57:59Z

@sethkinast can you take another look? You are right that using Chunk#await resulted in the context changing to the value of the resolved promise which is not desirable. I think I have that addressed now, but let me know what you think.

sethkinast

Looks good! A little change and then if you'll squash up your commits, I think we'll be ready to go

sethkinast · 2016-11-08T23:04:46Z

lib/dust.js

      });
    });
  };

  /**
+   * Render an error body if available
+   * @param thenable {Thenable} the target thenable to await


wrong param

sethkinast · 2016-11-08T23:05:04Z

lib/dust.js

+   * @param bodies {Object} may optionally contain an "error" which will be rendered
+   * @return {Chunk}
+   */
+  Chunk.prototype.renderError = function(err, context, bodies) {


Nice, good reusable func

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/

sethkinast · 2016-11-08T23:08:28Z

lib/dust.js

@@ -861,6 +861,17 @@
    var body = bodies.block,
        skip = bodies['else'];

+    if (dust.isThenable(elem)) {


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.

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

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

…e used when also using a thenable. The mapThenable function helps to resuse code in Chunk#await, Chunk#exists, and Chunk#notexists. It was also updated to capture and log errors similar to how Chunk#map does (seems to be a desired behavior that was missing when using thenables). Also fixed incorrect javadoc param on renderError(...).

samuelms1 · 2016-11-09T07:04:15Z

lib/dust.js

@@ -725,6 +725,11 @@
    return this;
  };



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.

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.

Note that this does slightly alter behavior in that a log is always generated on error rather than the previous logic that would only log an error when an error body was not present.

sethkinast · 2016-11-09T18:27:25Z

lib/dust.js

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

+    if (dust.isThenable(elem)) {
+      return mapThenable(this, elem, context, bodies, function(chunk, data){


Fantastic. Style nit: space between function(chunk, data) and its brace

sethkinast · 2016-11-09T18:30:43Z

Great work! If I can hassle you to fix the brace nit (which I should add to the linter...) we're ready to roll.

Also shoutout for writing great function documentation.

…race

samuelms1 · 2016-11-09T19:01:38Z

lib/dust.js

-            dust.log('Unhandled stream error in `' + context.getTemplateName() + '`', INFO);
-          }
+          chunk.renderError(err, context, bodies);
+          dust.log('Unhandled stream error in `' + context.getTemplateName() + '`', INFO);


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.

sethkinast · 2016-11-09T19:15:51Z

Thanks again for the PR! 🎆

samuelms1 · 2016-11-09T21:24:40Z

@sethkinast it was my pleasure. It was a lot of fun. Makes me want to contribute more in open source projects.

What is the release cycle for these changes? I'd like to start using them as soon as they become available.

sethkinast · 2016-11-09T21:30:47Z

I'm tempted to cut a 2.8, which I didn't plan on before a 3, and spend 3.0 working on an ES6 rewrite.

You could use the changes today by putting a commit hash in your package.json instead of a version number, if you wanted.

samuelms1 · 2016-11-10T07:14:53Z

@sethkinast We don't reference dustjs-linkedin directly or I might consider that. We use hoffman for the expressjs integration it provides. hoffman uses duster and duster uses dustjs-linkedin.

The ES6 rewrite sounds like a fun project to work on. Is there a branch for it?

sethkinast · 2016-11-10T19:32:08Z

I have not pushed a public branch for ES6 yet but hopefully soon.

Let me see about cutting a 2.8 in the next week or two.

…ts (linkedin#753) Closes linkedin#752

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

7a120e6

samuelms1 mentioned this pull request Nov 7, 2016

{?exists/} behavior different when using promises #752

Closed

samuelms1 closed this Nov 7, 2016

Samuel Smith added 2 commits November 8, 2016 11:48

Updated templates test so that it correctly reports errors as "expect…

71fdcdc

…ed {expected} to equal {actual}" rather than vice versa

Updated {?exists} and {^exists} logic to support promises and report …

dcc36d0

…errors. Previous implementation had an issue where the context was updated to the resolved promise data which was not intended as sethkinast pointed out.

samuelms1 reopened this Nov 8, 2016

sethkinast reviewed Nov 8, 2016

View reviewed changes

samuelms1 commented Nov 9, 2016

View reviewed changes

Samuel Smith added 2 commits November 9, 2016 00:27

Updated Chunk#stream to use Chunk#renderError.

6cc5a3c

Note that this does slightly alter behavior in that a log is always generated on error rather than the previous logic that would only log an error when an error body was not present.

Removed unnecessary semicolon

337ef1a

sethkinast reviewed Nov 9, 2016

View reviewed changes

Updated spacing so that functions have a space before opening curly b…

c7a4952

…race

samuelms1 commented Nov 9, 2016

View reviewed changes

sethkinast merged commit d485a72 into linkedin:master Nov 9, 2016

samuelms1 deleted the exists-supports-promised-values branch April 20, 2017 18:30

samuelms1 mentioned this pull request Sep 13, 2017

Dust 2.8 #757

Open

5 tasks

jrrbru pushed a commit to jrrbru/dustjs that referenced this pull request Nov 25, 2019

{?exists} and {^exists} resolve Promises and check if the result exis…

6e5445f

…ts (linkedin#753) Closes linkedin#752

snyk-bot mentioned this pull request Dec 30, 2021

[Snyk] Security upgrade dustjs-linkedin from 2.7.5 to 3.0.1 Natural-Intelligence/dustjs#4

Open

captainbarber99 mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade dustjs-linkedin from 2.7.5 to 3.0.1 captainbarber99/dustjs#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

samuelms1 commented Nov 7, 2016

sethkinast commented Nov 7, 2016 •

edited

Loading

sethkinast commented Nov 7, 2016

samuelms1 commented Nov 8, 2016

sethkinast commented Nov 8, 2016

samuelms1 commented Nov 8, 2016

sethkinast left a comment

sethkinast Nov 8, 2016

sethkinast Nov 8, 2016

samuelms1 Nov 9, 2016

sethkinast Nov 8, 2016

sethkinast Nov 8, 2016

samuelms1 Nov 9, 2016

samuelms1 Nov 9, 2016 •

edited

Loading

sethkinast Nov 9, 2016

sethkinast Nov 9, 2016

sethkinast commented Nov 9, 2016

samuelms1 Nov 9, 2016

sethkinast commented Nov 9, 2016

samuelms1 commented Nov 9, 2016

sethkinast commented Nov 9, 2016

samuelms1 commented Nov 10, 2016

sethkinast commented Nov 10, 2016

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

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

Conversation

samuelms1 commented Nov 7, 2016

sethkinast commented Nov 7, 2016 • edited Loading

sethkinast commented Nov 7, 2016

samuelms1 commented Nov 8, 2016

sethkinast commented Nov 8, 2016

samuelms1 commented Nov 8, 2016

sethkinast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelms1 Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkinast commented Nov 9, 2016

Choose a reason for hiding this comment

sethkinast commented Nov 9, 2016

samuelms1 commented Nov 9, 2016

sethkinast commented Nov 9, 2016

samuelms1 commented Nov 10, 2016

sethkinast commented Nov 10, 2016

sethkinast commented Nov 7, 2016 •

edited

Loading

samuelms1 Nov 9, 2016 •

edited

Loading