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

Prioritize resolution by .then for thenable functions #735

Closed
brianmhunt opened this issue Jun 9, 2016 · 4 comments
Closed

Prioritize resolution by .then for thenable functions #735

brianmhunt opened this issue Jun 9, 2016 · 4 comments

Comments

@brianmhunt
Copy link
Contributor

brianmhunt commented Jun 9, 2016

Here are a couple simple changes to make function-thenables work as expected and increase compatibility with e.g. Knockout:

1. Change dust.isThenable to

  dust.isThenable = function(elem) {
    return elem &&
           (typeof elem === 'object' || typeof elem === 'function') &&
           typeof elem.then === 'function';
  };

This matches Promises A+ spec, which states "1.2 “thenable” is an object or function that defines a then method."

(As a total aside, you may want to test that elem !== null since typeof null is object and null.then will throw an exception.)

2. Towards the end of Context.prototype._get change:

  • if (typeof ctx === 'function') { to
  • if (typeof ctx === 'function' && !dust.isThenable(ctx)) {

So if a "thenable function" (a function with a .then) is passed in, it prioritizes resolution by via the .then over calling the function.

3. In Chunk.prototype.reference move:

if (dust.isThenable(elem)) {
  return this.await(elem, context, null, auto, filters);
}

to the beginning of the function (i.e. before if (typeof elem === 'function') {).

4. Add a test, along these lines (pardon the style/naming):

describe("references ...", function () {
  it("dereferences via .then on thenable functions", function () {
    var fn = function () { return 'no' }
    fn.then = (res) => "yes"
    dust.renderSource("a{x}b", {x: fn}, (err, out) => assert.equal(out, "ayesb"))
  })
})

Use case

If you are using Knockout.js (and lots of folks do to) then most view-models passed in will use knockout observables, and look like this:

var vm = {
   x: ko.observable()
}

These observables are functions; you call them to read the value, write to them by passing the new value as the argument to the same function i.e. x(123) sets the value to 123; x() then returns 123.

When you call dust with this with e.g. dust.renderSource("{x}", vm, ...) it modifies the observable, which is undesirable. (It becomes the chunk via ctx.apply(ctxThis, arguments); in Context.prototype._get.)

Prioritizing thenable over functions makes it trivial for Knockout to expose its value. (For interests sake, until it's built in, KO users need to add something like this: ko.subscribable.fn.then = function(res) { res(this()) }). I also feel like this is the more natural prioritization.

I doubt many folks will have thenable functions, so I suspect this change has a low probability of breakage – and where folks do have thenable functions I suspect the preference is to resolve via the then as opposed to a function call.

Thanks for reading; I hope this proves to be a useful request. Happy to put together a PR, but I just posted here since the changes seem pretty simple.

Cheers.

@brianmhunt brianmhunt changed the title Better isThenable (slightly) Prioritize resolution by .then for thenable functions Jun 9, 2016
@brianmhunt
Copy link
Contributor Author

I just noted at least one other place where a small change is required, namely Chunk.prototype.section. Will look for and post about others.

brianmhunt added a commit to brianmhunt/dustjs that referenced this issue Jun 9, 2016
@sethkinast
Copy link
Contributor

Interesting, thanks so much for the thorough overview. I see you're working on a PR; I'm totally on-board with pulling this in when you're ready.

@sethkinast
Copy link
Contributor

As an aside, how does one use Knockout with Dust, anyways? I would have assumed that Knockout handled all the templating.

@brianmhunt
Copy link
Contributor Author

Awesome. Thank you so much for receiving and considering the PR, which is now up as #736.

As an aside, how does one use Knockout with Dust, anyways? I would have assumed that Knockout handled all the templating.

Knockout is essentially template-agnostic, but it's much easier to stick with the default templating system. The data/observable model, however, can be completely severed from Knockout.†

† I am in the process of breaking Knockout out into reusable parts now (per, among others, tko.observable).

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

No branches or pull requests

2 participants