ufront / ufront-mvc

The core MVC framework that powers ufront
MIT License
17 stars 15 forks source link

Add async View/Json/RedirectResult #15

Closed kevinresol closed 9 years ago

kevinresol commented 9 years ago

Similar to ViewResult but accept a Future<TemplateData> instead of TemplateData. So this will work well with AsyncApi.

example:

@:route(GET, "/profile/$id")
public function profile(id:Int):ActionResult
{
    var templateDataFuture = asyncAuthApi.getUser(id).map(function(user):Dynamic return {profile:user.profile});

    return new AsyncViewResult(templateDataFuture);
}

Note: we can't create closures calling super.method, so I moved the code to internalExecuteResult()

kevinresol commented 9 years ago

I think there should be async versions for all other ActionResults, then the UFControllers would work perfectly with asyncApis.

e.g.

@:route(POST, "/test")
public function test(args:{...}):ActionResult
{
    var future = asyncApi.doSomething();

    var futureTemplateData = future.map(function(_) return {key:"id", value:1});
    return new AsyncViewResult(futureTemplateData);

    // or 
    var futureObject = future.map(function(_) return {key:"id", value:1});
    return new AsyncJsonResult(futureObject);

    // or 
    var futurePath = future.map(function(_) return "/redirect/to/here");
    return new AsyncRedirectResult(futurePath);

    // etc...
}
jasononeil commented 9 years ago

I agree with the idea, but wonder if we can use the Futuristic type for this.

That way you could use both:

var syncData = { page: 1 };
var futureData = Future.sync({ page: 2 });
new JsonResult( syncData );
new JsonResult( futureData );

The main downside is that it will wrap any synchronous results in a future, which is a slight amount of indirection, but I doubt will have any significant performance issues. It may also make unit testing slightly more difficult.

The main upside is that we get to keep just one API for each type of result. Which is less confusing and easier to maintain.

kevinresol commented 9 years ago

Didn't know there is the Futuristic type before. I agree it is better to have only one version for each type of result.

jasononeil commented 9 years ago

I should be able to pull these together fairly quickly. Do you think the extra parameters need to be Futuristic as well?

To use RedirectResult as an example:

public function new( url:Futuristic<String>, permanentRedirect:Bool )
or
public function new( url:Futuristic<String>, permanentRedirect:Futuristic<Bool> )

I might just start with the main piece of data for each action type, and we can support the others later if we want, because the change is unlikely to break anybody's code.

kevinresol commented 9 years ago

Oh sorry but I just noticed there is an FutureActionResult class. Does it mean the controller functions can actually return a Future<ActionResult> in place of ActionResult? If that is the case, we don't even need to change the Result classes. We just do:


@:route(GET, "/whatever")
public function whatever()
{
  return Future.sync(new AnyActionResult());
}
kevinresol commented 9 years ago

If currently controller functions does not support returning FutureActionResult, will you consider adding it? I think the effort will be smaller than changing all the ActionResult subclasses one by one.

jasononeil commented 9 years ago

Controllers do support async values already, basically Controller.execute() will accept a return value of:

I thought I'll write this out in full because I'll add it to the documentation I'm writing.

But essentially yes, we could use that rather than changing all of our content results. I don't know why I didn't suggest that earlier...

jasononeil commented 9 years ago

Do you think that's an adequate solution or would you still prefer to add Futuristic support to our ActionResult classes? I actually already started (derp...) and it's not too hard.

kevinresol commented 9 years ago

Oh then that is just an alternative so that you can do both return Future.sync(new SomeActionResult(data)) and return new SomeActionResult(Future.sync(data))

I think it benefits the users anyway

jasononeil commented 9 years ago

Alright, I've updated ViewResult, RedirectResult and ContentResult to all accept async data as well.

So now you can use them either way:

@:route(/home/) function homepage() {
    var dataFuture = asyncApi.getPageData();
    // Option A:
    return dataFuture >> function( data:TemplateData ) return new ViewResult( data );
    // Option B:
    return new ViewResult( dataFuture );
}

This prompted me to do a fairly significant cleanup of ViewResult as well - it was pretty ugly before. Hopefully less ugly now.