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

Output of render should be Promise-compatible #251

Closed
DylanPiercey opened this issue Mar 20, 2016 · 7 comments
Closed

Output of render should be Promise-compatible #251

DylanPiercey opened this issue Mar 20, 2016 · 7 comments
Assignees
Labels
good first issue Small tasks that would be good for first time contributors scope:runtime type:feature A feature request
Milestone

Comments

@DylanPiercey
Copy link
Contributor

Currently rendering a template returns an AsyncWriter, it would be handy though if when a callback is omitted that templates render function returned a promise.

var template = require("template.marko");

template.render({ hello: "world" })
    .then(function (html) { ... })
    .catch(function (err) { ... });

If you're using promises already it makes using marko a bit easier.

@GenaANTG
Copy link

GenaANTG commented Jul 2, 2016

You can use temporarily bluebird module.

let Promise = require('bluebird');
let EntryCommentsMarkup = require('./views/EntryComments.marko');
Promise.promisifyAll(EntryCommentsMarkup);

then in routing we invoke renderAsync method:

app.get('/example', function(req, res) {
    let Data = { ... };
    return EntryCommentsMarkup.renderAsync(Data)
        .then((markup) => res.status(200).json({ markup: markup }))
        .catch((error) => res.status(400).json({ message: error.message }));
});

@patrick-steele-idem
Copy link
Contributor

/CC @philidem @mlrawlings

I would like to see a method that returns a promise, but I would prefer to avoid a breaking change. The purpose of returning the AsyncWriter instance was to be compatible with the EventEmitter API (on, once, emit, etc.) that AsyncWriter implements. For example:

template.render({ hello: "world" })
    .on('foo', function(e) {

    })
    .on('error', function(e) {

    })
    .on('finish', function() {

    })

What are your all thoughts on utilizing the native promise implementation to provide a new renderAsync method?:

var template = require("template.marko");

template.renderAsync({ hello: "world" })
    .then(function (html) { ... })
    .catch(function (err) { ... });

@mlrawlings
Copy link
Member

mlrawlings commented Jul 14, 2016

I'd rather modify async-writer to be a thenable by adding .then and .catch

I've seen other libraries such as superagent and mongoose implement a promise-like interface that plays well with anything that is duck-typing rather than looking at instanceof Promise (which is most everything due to things like bluebird and q).

It would support both

template.render({ hello:"world" })
    .then(function (html) { ... })
    .catch(function (err) { ... });

and

template.render({ hello:"world" })
    .on('error', function(e) { ... })
    .on('finish', function() { ... })

And with async/await you'd be able to do:

var html = await template.render({ hello:"world" })

We could also implement a .promise function (or some other name) that returns a true Promise for cases where it is needed.

Expand for possible implementation

{
    then(fn, fnErr) {
        var promise = new Promise((resolve, reject) => {
            var buffer = ''
            this.on('data', data => buffer += data)
            this.on('error', error => reject(error))
            this.on('finish', () => {
                try {
                    resolve(fn(buffer))
                } catch(err) {
                    reject(err)
                }
            })
        })
        if(fnErr) {
            promise = promise.catch(fnErr)
        }
        return promise
    }
    promise() {
        return this.then(x => x)
    }
    catch(fn) {
        return this.promise().catch(fn)
    }
}

@patrick-steele-idem
Copy link
Contributor

I like the suggestion to implement the then and catch methods on the AsyncWriter instance. Let's go forward with that approach.

@mlrawlings mlrawlings added the good first issue Small tasks that would be good for first time contributors label Jul 15, 2016
@patrick-steele-idem patrick-steele-idem added this to the 4.0 milestone Nov 8, 2016
@patrick-steele-idem patrick-steele-idem changed the title Return promise when callback omitted on render. Output of render should be Promise-compatible Nov 8, 2016
@patrick-steele-idem
Copy link
Contributor

FYI: it should be sufficient to add the then() and catch() methods to OutMixins.js since those mixins are added to both AsyncStream and AsyncVDOMBuilder

@patrick-steele-idem
Copy link
Contributor

NOTE: We want to follow the rules for having a consistent rendering API: #389

That is:

template.render({})
    .then(function(out) {
        out.appendTo(document.body);
    });

austinkelleher added a commit to austinkelleher/marko that referenced this issue Nov 11, 2016
austinkelleher added a commit to austinkelleher/marko that referenced this issue Nov 11, 2016
austinkelleher added a commit to austinkelleher/marko that referenced this issue Nov 11, 2016
@Hesulan
Copy link
Contributor

Hesulan commented Nov 12, 2016

A bit late, just throwing this out there: The Promises/A+ specification requires that if the result is an object or function with a then method, it must be treated as a "thenable". In other words, this solution is explicitly permitted by the specification, and any Promise library which doesn't handle this correctly is - by definition - "broken". That's all, carry on. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Small tasks that would be good for first time contributors scope:runtime type:feature A feature request
Projects
None yet
Development

No branches or pull requests

6 participants