StubbleOrg / Stubble

Trimmed down {{mustache}} templates in .NET
Other
401 stars 58 forks source link

Async/Await token function? #88

Closed alexdresko closed 4 years ago

alexdresko commented 4 years ago

I apologize if I got the terminology wrong. I'm finding that it's not possible to have a token function that returns a Task<object>. So in this example, the TestMethodAsync token isn't working at all (although it is being stripped from the template).

async Task Main()
{
    dynamic data = new ExpandoObject();

    data.TestProperty = "Alex is cool!";
    data.TestMethod = new Func<string, object>(s => s.ToUpper());
    data.TestMethodAsync = new Func<string, Task<object>>(
        async s =>
        {
            await Task.Delay(TimeSpan.FromMilliseconds(100));
            return s.ToLower();
        });

    var template = "Hello, World! {{TestProperty}} {{#TestMethod}}Look up!{{/TestMethod}} {{#TestMethodAsync}}lOoK DoWn!{{/TestMethodAsync}}";

    var stubble = new StubbleBuilder().Build();

    var result = await stubble.RenderAsync(template, data);

    // Expected: Hello, World! Alex is cool! LOOK UP! look down!
    // Actual: Hello, World! Alex is cool! LOOK UP! lOoK DoWn!
    (result as string).Dump();
}

LINQPad demo here

Romanx commented 4 years ago

Hi there,

Don't worry about the terminology. It looks like what's happening here is that our lambda function implementation does not handle Tasks (so does not await them). This means that it's being seen as a truthy check on a value which is true since the method does exist and so the content is being rendered.

The reason we don't await the lambda is mostly due to the fact we try to stick to mustache's core philosophy of being logic-less and having this await the result of the method while being valid would lead people to possible perform HTTP or DB calls in such methods and would impact render time and would not be immediately obvious.

My advice would be to perform any async lookups before rendering and map to another object (a view model or dto like object) for rendering thus avoiding the async outside of the template. The reason we provide the RenderAsync is so that templates could be looked up asynchronously however we recommend they are then cached.

Hope this helps some,

alexdresko commented 4 years ago

The reason we don't await the lambda is mostly due to the fact we try to stick to mustache's core philosophy of being logic-less and having this await the result of the method while being valid would lead people to possible perform HTTP or DB calls in such methods and would impact render time and would not be immediately obvious.

I understand if you don't want to change the implementation, but, personally, I'm okay with the render time being slowed due to the work I'm doing in my lambda functions. Plus, just because a lambda returns Task doesn't mean it's a long-running operation. Also, developers can still perform long-running operations in lambda functions that don't support Task.

My advice would be to perform any async lookups before rendering and map to another object (a view model or dto like object) for rendering thus avoiding the async outside of the template. The reason we provide the RenderAsync is so that templates could be looked up asynchronously however we recommend they are then cached.

I can't do my lookups before, because that would impact performance. Instead of pre-populating all of our data, I only have to populate for the tokens that were provided in the template.

Is there any way this feature could be added as a "use at your own risk" kind of thing?

Romanx commented 4 years ago

Hi there,

I've had a thought about this and for async rendering I agree it doesn't make sense the async lambdas aren't handled correctly. This will make both the sync and async paths act slightly differently but in this case I'd argue that would be expected but we should document it. I'll mark this as a feature and hopefully get to it by the weekend.

Thanks for reporting and using Stubble!

alexdresko commented 4 years ago

Cool! Looking forward to the enhancement. I'll be happy to report my findings.

Romanx commented 4 years ago

Hi there,

This has now been completed and will be released shortly. Please let me know if you have any issues. Thanks for using Stubble!

alexdresko commented 4 years ago

Well, all of our unit tests are passing after updating the nuget package and making everything async!!

Thanks so much! This is a pretty big/important change for us.

Romanx commented 4 years ago

You're welcome, I'm glad we could help.