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

Add deep resolution of Thenables in context #590

Merged
merged 4 commits into from
Mar 26, 2015

Conversation

sethkinast
Copy link
Contributor

As part of this change, Jasmine has been updated to properly handle async render tests. Before this, any asynchronous test that called dust.render() always passed.

  • Updated grunt-contrib-jasmine to use Jasmine 2.0.1 (was 1.3.1)
  • Updated Rhino test JAR from 1.7R5-rc1 to 1.7R5-final
  • Refactored unit tests for Jasmine 2

Closes #588

@sethkinast sethkinast force-pushed the rhino branch 2 times, most recently from 9a0085e to 2089482 Compare March 25, 2015 08:25
Previously tests that called dust.render and had an async component (like chunk.map) would always pass.

* Upgrade jasmine to 2.0.1 and refactor unit tests for compatibility
* Upgrade Rhino JAR from 1.7R5-rc to 1.7R5-final
@sethkinast sethkinast force-pushed the rhino branch 2 times, most recently from 5cbbc68 to 9198126 Compare March 25, 2015 20:30
@sethkinast
Copy link
Contributor Author

Reworked the PR into two separate commits so it was easy to see the changes needed to support deep resolution.

@jimmyhchan
Copy link
Contributor

The async commit is awesome. Thanks for that. The deep promise resolution hurts my brains.

Please add a test in case one of the thenable thens (I N C E P T I O N) fails.

As discussed a partial function here would make this easier to grok. The closure to get at currentContext and tracking down down.slice(i)only to realize that it stays as i since you return immediately make go way over my head. (stupid promises and their single return value :) )

Nice to have but not necessary

If you have a Thenable `{foo}` that resolves with `{"bar": "baz"}`, you can now put `{foo.bar}` in a template and get "baz".
@sethkinast
Copy link
Contributor Author

Added a third commit with an explicit closure creation. Let me know if you like that better.

I think I don't like it as well just because it's one extra layer of indirection for me to keep track of in my mental model. I can see how it makes it more explicit, though.

@sethkinast
Copy link
Contributor Author

Added some new tests, by the way. How sick is this:

      {
        name:     "thenable deep section, traverse outside",
        source:   "Eventually my {#magic.ally}{prince} {delicious}{/magic.ally} will come",
        context:  { "prince": "Prince", "magic": new FalsePromise(null, {"ally": {"delicious": new FalsePromise(null, "Lucky Charms")} }) },
        expected: "Eventually my Prince Lucky Charms will come",
        message: "should reserve an async section for a deep-reference thenable and not blow the stack"
      },

Half of the section key lookup is a promise and the other half isn't.

@jimmyhchan
Copy link
Contributor

jimmyhchan commented Mar 26, 2015 via email

@sethkinast
Copy link
Contributor Author

That's what chunk.map and friends are for. All of this is just syntactic sugar.

At Amazon we had a wrapper around chunk.map to make it easier to use that looked something like this:

return async(arguments, function(params, done) {
    // do some async stuff, then eventually
    done(null, { some: "data" }); // pushes onto the context and calls chunk.render for you, etc
});

@sethkinast
Copy link
Contributor Author

You can definitely return a promise from a helper now, that's maybe 60% the point of this PR and the one before it, so that people can plop externalModule.doSomethingThatReturnsAPromise into their context and it just works.

{
  "user": db.fetchUser
}

vs

{
  "user": function(chunk, context, bodies, params) {
    return chunk.map(function(chunk) {
      db.fetchUser().then(function(data) {
        chunk.render(bodies.block, context.push(data)).end();
      }, function(err) {
        chunk.render(bodies.error, context.push(err)).end();
      });
    });
  }
}

prashn64 added a commit that referenced this pull request Mar 26, 2015
Add deep resolution of Thenables in context
@prashn64 prashn64 merged commit 2141106 into linkedin:master Mar 26, 2015
@sethkinast sethkinast deleted the rhino branch March 26, 2015 20:13
@sethkinast sethkinast restored the rhino branch March 29, 2015 04:32
@sethkinast sethkinast deleted the rhino branch April 1, 2015 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deep-resolve thenable values
3 participants