Made controller lazy load.

[alexindigo]

Spike,

This is step 2 towards RequireJS support.
For now I require views and compiled templates from within controllers,
but it's in my plans to make view/template async load part of Rendr too.
(Again aiming for minimal changes to Rendr).

/сс @jskulski @bigethan @c089

[spikebrehm]

Little typo here

[alexindigo]

Not anymore :)

[spikebrehm]

There's some duplication here that would be nice to DRY up. Could add a function that calls the action.

function callAction(action) {
  if (!action) {
    throw new Error("Missing action \"" + route.action + "\" for controller \"" + route.controller + "\"");
  }
  action.call(router, params, router.getRenderCallback(route));
}

then use it in both code paths like:

callAction(action);

[alexindigo]

I didn't want to have extra function expression within action handler, it might be not a big deal on the client,
but on the server it will cause unnecessary slow down.

So it's a little bit wet.

actionCall(action, params);

[spikebrehm]

Interesting. Seems reasonable but again it would be really nice to have a little example app so we can play around with the changes before committing blindly.

[alexindigo]

For now to make our app run it requires manual hacks and adjustments, since not all pieces are in place (i.e. RequireJS bug, pathAdapter for Rendr, views/templates loaded within controller) – I'm not if demo like that would be helpful or just confuse other parties. (When all that is done, I'm going to submit PR with requirejs demo).

@c089 was going to provide their browser test scripts where we can see how it performs with both Stitch and RequireJS.

But if you're worried that we're going the right direction, I'll setup playground where you can see our changes in action.

[c089]

the err msg code is duplicated from above

[alexindigo]

Yeah, but it's just error message.

[c089]

It's still duplication and really trivial to avoid using the extract method refactoring.

[alexindigo]

I agree, but then it asks for the error handling subsystem, which is probably welcome change, but not in this PR.

[c089]

To be honest I'm not too much of a fan of this. Seems like this will change things all over the place, mixing concerns, in this case routing and AMD support and making the code much harder to understand. I don't know enough about the different module systems to provide a better solution though.

[c089]

Quick thought: Could we maybe solve this in a different way altogether if we had a new design of the routing? I think i might have have to change substantially anyway to solve issues with express vs. backbone router with the goal being to implement a new isomorphic routing module...

[alexindigo]

I'm ok with anything that allows async controller load. Do you have example of the new routing system?

[alexindigo]

@spikebrehm Here is demo app (05_requirejs), it's still has bunch of hacks/workaround, since not all the planned changes been coded/landed into Rendr.

But I hope it will give better understand on what we're trying to do.

PS. It works in non-minified mode, idea to have separate grunt task that will minify/combine certain modules and lazy load other, similar to how libs.js is combination of all third-party libs/modules.

If you have more question, let's talk in IRC.

Thank you.

[alexindigo]

@spikebrehm Have you had a chance to look at the new changes?
And I have final changes in the pipe line, that allow demo app run with no hacks.

Thank you.

[spikebrehm]

@alexindigo sorry I haven't had a chance to look; heads down trying to get a product out by the end of the week. I'll try to take a peek but my SLA will be poor.

[alexindigo]

There is new PR with latest changes to Rendr and demo app – #190
Thank you.