Reactivity breaks contentFor

[tmeasday]

Reproduction:

1. Clone https://github.com/tmeasday/ir-reactivity-problem, run against iron-router
2. Open in browser, change the 'foo' session variable.
3. You should see:

Session.set('foo', 'bar')
undefined
Exception from Deps recompute: Error: You called contentFor outside of a        Controller._CurrentPartials.withPartials callback
    at assert (http://localhost:3010/packages/controller/lib/controller.js?eea9e49991ca8bc77186103b536d7d8378591fc1:3:11)
    at Object.Helpers.contentFor (http://localhost:3010/packages/controller/lib/controller.js?eea9e49991ca8bc77186103b536d7d8378591fc1:55:5)
    at apply (http://localhost:3010/packages/handlebars/evaluate.js?742a34cf3479d95600582786665b53c0eba4f7d9:232:24)
    at invoke (http://localhost:3010/packages/handlebars/evaluate.js?742a34cf3479d95600582786665b53c0eba4f7d9:257:12)
    at http://localhost:3010/packages/handlebars/evaluate.js?742a34cf3479d95600582786665b53c0eba4f7d9:341:27
    at Object.Spark.labelBranch (http://localhost:3010/packages/spark/spark.js?4af332696fb84f1c71f2e678ad0a267755b2b828:1114:14)
    at branch (http://localhost:3010/packages/handlebars/evaluate.js?742a34cf3479d95600582786665b53c0eba4f7d9:311:20)
    at http://localhost:3010/packages/handlebars/evaluate.js?742a34cf3479d95600582786665b53c0eba4f7d9:340:20
    at Array.forEach (native)
    at Function._.each._.forEach (http://localhost:3010/packages/underscore/underscore.js?ed2d2b960c0e746b3e4f9282d5de66ef7b1a2b4d:78:11) 

[cmather]

There are two issues here:

1. The error you're seeing above is because the original implementation of contentFor checks Controller._CurrentPartials.get() to get the current partials object. But when Spark calls the child template function again, it is not called within a Controller._CurrentPartials.withPartials block. Upon reflection, I realized the partials check is an unnecessary restriction. If there is no partials object then contentFor and yield should just be noops. See: https://github.com/EventedMind/meteor-controller/commit/fc3fb4183186cc12c0477247df1219b0aea92842
2. But there is another bug which can be seen in the following example:

<template name="layout">
 <aside>{{{yield "aside"}}}</aside>

{{{yield}}}
</template>

<template name="child">
  {{#contentFor "aside"}}
    {{asideContent}}
  {{/contentFor}}

  {{templateContent}}
</template>

1. Session.set('asideContent', 'this works'); - The aside content properly re-renders inside the layout
2. Session.set('templateContent', 'this works'); - The child template is re-rendered successfully
3. Session.set('asideContent', 'broken'); - Somehow the reactivity is broken now

I would expect the original {{{yield "aside"}}} to maintain its reactivity because it has its own isolate annotation. This is why step 1 above works. When the child template re-renders, it shouldn't affect the parent live range. So when the child template re-renders, my guess is somehow the original function inside the layout aside isolate is destroyed and doesn't fire anymore. But this is a tired guess :-). More tomorrow.

[tmeasday]

The reason for this is as follows, AFAICT:

When {{#contentFor}} is called, the controller creates a isolate block, from inside the computation of the child template.

That isolate block behaves properly, but when the child computation is invalidated (point 2.), it is cleaned up automatically by Meteor, and thus we lose reactivity at 3.

A workaround that solves this particular issue is to setup the isolate block when {{yield}} is called (and thus within the layout's computation): https://github.com/tmeasday/meteor-controller/compare/attempted-fix?expand=1

However, this just shifts the issue: now if we add something reactive to the layout, then the isolate will get killed when the layout re-renders.

I pushed a version of ir-reactivity-problem to demonstrate this.

I don't really know how to work around this problem. It feels "incorrect" to me that when {{#contentFor}} is re-run it is a no-op; but obviously I can understand why it needs to be. Can we annotate the HTML with a link to the partials somehow and get them back out in the helper?

[cmather]

Yeah, this is exactly right. Thanks for clarifying this for me. I need to think about this for a bit.

[tmeasday]

My only other comment would be that I don't see a huge problem with yields being global--yes it would be nicer for them to be scoped as they are right now, but if we need to sacrifice that to get them working, I think it's OK.

[cmather]

I just spent the last few hours studying how Ember does this and took away some interesting insights. Also, took away some thoughts about where David is going with the UI packages (take a look at component.js, similar to a View in Ember). In any case, in Ember, there are actually two ways to control rendering: yield (part of a view) and outlets (part of routing). If we adopted something similar to outlets, it solves the problems of breaking up a template into a layout and child. But would once again change the way router works (the plumbing, not so much the API). So, I need to think about this some more and I'll clarify what I come up with.

[tmeasday]

Ok, I'll take a look at ember myself in a little bit.

[cmather]

http://emberjs.com/guides/routing/defining-your-routes/

[cmather]

Below is a summary of my takeaways and some ideas that might apply here:

Ember outlets and yields

outlets and named outlets In Ember, a route can call render multiple times. Each render method takes a templateName as a required parameter, but you can also specify an outlet to render in to.

this.render('post/new'); // by default render into the main outlet of the parent (Ember has a hierarchy of routes)
this.render('post/new/aside', {outlet: 'aside'}); // this renders a different template into the 'aside' outlet of the layout template. But aside stays as a separate template.

yield In Ember, yields are only used in a View which is very similar to a Meteor Component - see the shark branch and look at the UI package.

Ember simplifies all of the issues we're running into by not allowing named yields. They're primarily for rendering content into a layout for a given component - like a popup window or dialog box for example.

{{{yield}}} in Ember has nothing to do with Routes.

Conclusion and takeaways

• Ember's idea of a hierarchy of routes is interesting but probably requires more debate in our context.
• But the key simplification that still meets our use case, is that a Route can call render multiple times, "placing" templates into a parent template's "outlets." This alleviates the issue of losing reactivity, events, etc. on the templates.
• Ember maintains automatically creating a route class if one doesn't exist. It maps a named route to a route class. The render method and renderTemplate method (same as our run method) are implemented in the route class itself. Route classes are singletons; no instances are created.
• I'm going to play with rendering into an outlet without re-rendering the parent each time.
• This might make your transition use case much simpler to deal with.

[tmeasday]

Phew! Just did an intense session of reading through the ember docs (which I should have done a little while ago probably).

Thoughts on outlets

1. I think they are cool, and it's a better solution to use a template than {{#contentFor}}.
2. I think it should be done by default at the controller level, rather than route level though:

PostController = RouteController.Extend({
  template: 'postsShow',
  layout: 'layoutWithAnAside',
  outletTemplates: {
    aside: 'postsShowAside'
  }
});

Perhaps the route can override this, but I think the controller is the right place to talk about this stuff (assuming the controller is setting it's own layout anyway). Ember doesn't have the concept of layouts, which is why it makes more sense to talk about it at the route level.

But I think this puts far too much presentation logic into the router.

1. An important difference related to the above. I think ember wants you to use a different controller for the outlet? It definitely lets you. This isn't what I need for my use case, I need it to be the same controller. Probably we don't need to support it right away, anyway.
2. I'm not quite seeing how it helps me with my transition stuff, but I'd love to hear it :)

Random thoughts of my own

1. Ember <=> Angular naming: route=route / controller=controller / view=directive
   • calling it a view was a very poor choice IMHO because the "controller" isn't a traditional controller and it's more like a ViewModel, or even a View in Django. So folks is going to get confused (I was confused).
2. Ember's route hierarchy is cool, but I agree, leave this for now.
3. Aside: I like how Ember has gone in an Angular-like direction in using named actions for event handling. I don't know if David is going here with components (I tried looking at the code in shark but it was pretty impenetrable).

Conclusion

If it's just a matter of a controller specifying templates for outlets rather than a template doing {{#contentFor}}, then it shouldn't be a big change and I say go for it :)

[cmather]

@tmeasday,

• I like the idea of being able to specify outlets in the controller definition.
• I think my initial design of Controller overreaches a bit in terms of completely wrapping a template (event handlers, custom landmark, custom callbacks, etc), especially in light of Meteor's work on the UI package. But still thinking this through.
• But, maybe the issue is just in layouts and named yields. I think yields belong in a View (like Ember). In Meteor, this would be the Component.
• In routing, using the concept of an outlet (and named outlets) and a 1-1 relationship between outlet and template greatly simplifies things.
• We're taking a gamble in introducing a new symbol and terminology - Controller. Maybe this is okay, as it looks like Meteor Component is all the functionality that Ember View has, implemented differently.
• I agree that Controllers in Ember are confusing and didn't make sense to me. Maybe it has more meaning in frameworks like Cocoa, iOS?

I think I need a break from this today :-). Going to get back to working on my course and make a few more videos.

[tmeasday]

More thoughts

• I agree that probably some of the concerns of the component has leaked into your controller. Specifics of rendering callbacks do probably belong at the template/component level and the controller itself can probably concern itself with higher level stuff.
• I feel the layouts around views in ember is incomplete in that it doesn't have named yields. In their very example, there's no way (I can see) to put a title on the popout, which is such a common thing to do. (http://emberjs.com/guides/views/adding-layouts-to-views/#toc_applying-layouts-in-practice)
• I think "Controller" makes sense as the interface between the model and the view, ala classic MVC. However: a) It's probably more correct to call it a Presenter (http://en.wikipedia.org/wiki/Model–view–presenter). Although I make no claim to be an expert on what the difference is. b) Their "Templates" are Views in the MVC sense (handlebars makes them code after all). Their "Views" are just like extended templates AFAICT, so I think they should have a different name ("Component" seems a good one--I think David got this right. Angular got it wrong too as "Directive" is just too vague).

What I'd like controllers to do:

In an ideal world, whatever they are called.

1. Render more than one template in a sensible way wrt events and helpers (ie. layouts and outlets, or whatever mechanism makes the most sense).
2. [This is secondary and can be talked about later]: Maintain non-persistent state. Ember does this by decorating the model with presentational variables. I'd argue it makes sense to do the same.
3. [Also secondary] Possibly respond to named events, in the same way that they do in Ember---Controllers don't know about clicks and .buttons, they know about more abstract events that are defined at the component level.

I'm getting away from the core issue so I'll stop there. I know this conversation is massively blowing out :)

I still stick by my conclusion of the previous comment but I'm open to conversation about it.

[tmeasday]

I had a simpler idea than full blown controllers that I thought I'd quickly implement and get your thoughts on:

https://github.com/tmeasday/meteor-layouts

Basically it does the outlet idea with a simple block helper. I think it retains the reactivity / event handler requirements that I need, although I haven't thought too much about isolation yet. It seems to avoid this particular bug.

It's an idea that harks back to my old idea of the template specifying the layout. Let me know what you think.

[cmather]

Haven't forgotten about this @tmeasday. Had to get back to working on the other parts of the next EventedMind, and doing course materials. Will get back to this project either over weekend or Monday.

[cmather]

@tmeasday, I read through your code and notes. I have a few design thoughts below and I'm going to work on implementing this tonight. I've made a few simplifications that should make this easier to reason about, and to implement. It should also fit closer with Meteor's work with Component.

Design Goals

1. User specifies an "application" or "layout" template for the router and can use the {{yield}} or {{yield 'name'}} helper inside the layout. Only one master layout is used for now.
2. The {{contentFor 'name'}} helper would no longer be used anywhere
3. A RouteController can call its render method one OR MORE times.
4. The render method takes an optional parameter to that specifies where to render the given template to inside the layout. If no option is provided, it renders to the main {{yield}}.
5. Defaults can be provided to the router or to a controller.
6. Yielded content is not cleared on each route run. It's the new route's responsibility to say what should be in each content area. Is this a good idea? Maybe we provide a this.clear('yieldName') or something? Not sure.

Design

1. The ClientRouter renders itself into the region or body as it did before.
2. But then it first renders its layout if its available. The {{yield}} helpers drop LiveRanges that can be used later when a Controller wants to populate one of them.
3. When a controller renders to a specific named yield, only that region is affected.
4. Yield regions are tracked by the router itself. We could also make them global, but then we'd have to verify that the layout is actually in the DOM. We might have to do this anyway. Maybe a flushLayout or something.

I'll work on a prototype of the changes tonight while I'm waiting on feedback. Not entirely sure yet whether this will simplify things or make it worse.

[cmather]

@tmeasday, It looks like the easiest way to do this is with a global reactive dictionary (yieldPartials which I'll stash somewhere), as you originally proposed. I'm going to head down that path. I think this should greatly simplify things.

[cmather]

I implemented a prototype of the new rendering process and it seems to work well and actually simplifies a lot. I'll finalize the implementation tomorrow and push to the layouts branch for feedback.

The key points in addition to above are:

1. yield will be a global helper
2. partials reactive dictionary is a singleton (not one per router)
3. Router renders the layout if there is one, controller actually does the rendering of the child templates
4. If not layoutTemplateName provided, we'll create an anonymous one
5. Key issue becomes setting the data context for each child template. I'm using a global for this. Below is the sample implementation:

_yieldPartials = new ReactiveDict;
__MAIN__ = '__main__';

_CurrentDataContext = (function () {
  var current = null;

  return {
    get: function () {
      return current;
    },

    withValue: function (value, fn) {
      var prev = current;
      try {
        current = value;
        fn();
        // flushing here causes the yield function to run
        // right away with the correct data context.
        Deps.flush();
      } finally {
        current = prev;
      }
    }
  };
})();

Handlebars.registerHelper('yield', function (name, options) {
  //XXX need to get the data context here
  if (arguments.length < 2)
    name = '__main__';

  function renderPartial () {
    var data = _CurrentDataContext.get() || {}
      , templateName
      , html;

    templateName = _yieldPartials.get(name);
    html = templateName ? Template[templateName](data) : '';
    return html;
  }
  return new Handlebars.SafeString(Spark.isolate(renderPartial));
});

Router = {
  layoutTemplateName: 'application',

  render: function () {
    var self = this
      , layoutTemplate = Template[self.layoutTemplateName];

    //XXX what if no layout is provided?
    // we need a default behavior here
    return layoutTemplate();
  },

  autoRender: function () {
    var self = this;
    document.body.innerHTML = '';
    document.body.appendChild(Spark.render(_.bind(self.render, self)));
  }
};

Controller = function () {
};

Controller.prototype = {
  render: function (templateName, options) {
    var data, to;
    options = options || {};
    to = options.to || __MAIN__;
    data = options.data || {};

    console.log('rendering ', templateName);
    _CurrentDataContext.withValue(data, function () {
      _yieldPartials.set(to, templateName);
    });
  },

  run: function () {
    this.render('home', {
      data: {
        homeContent: function () {
          return Session.get('homeContent');
        }
      }
    });
  },

  runTwo: function () {
    this.render('posts', {
      data: {
        postsContent: function () {
          return Session.get('postsContent');
        }
      }
    });

    this.render('postsAside', {
      to: 'aside'
    });
  }
};

Meteor.startup(function () {
  Router.autoRender();

  c = new Controller;
  c.run();
});

[tmeasday]

I thought a bit about this on a train journey and I really like how this is turning out. Some thoughts:

On layouts + yields.

Can we think about things like this?:

• A layout function is a function that returns (annotatedHtml, yielder)
  • yielder is a function that takes names + rendering function (e.g. templates) and is responsible for inserting the template at the correct point in the annotatedHtml (i.e. they are tightly bound together).
  • You could then make a function that turns a rendering function that includes calls to {{yield X}} into such a layout function. It would be much like your code above, but instead it would explicitly create it's own partials Dict and implicitly return it in the yielder.
• Controller.run can then get called with a yielder and Controller.render() can use yielder to insert content at the right points.
• Similarly, I could make my {{#withLayout}} helper (to be used generally within templates) easily using such a mechanism.
• Finally, the transitioner could work like so: In a layout, rather than {{{yield}}}, I call `{{{transitionedYield}}}. And I define it something like

transitionedYield = function() {
  var onLeftPane = true;
  // XXX: some autorun that talks to the router and knows when to switch between panes

  var leftPaneLiverange = //?;
  var rightPaneLiverange = //;

  registerYielder('__base__': function(renderer) {
    if (onLeftPane) {
      // kill old leftPaneLiverange
      // render renderer into leftPaneLiverange
    }
  );

  return leftPaneLiverange + rightPaneLiverange;
}

What do you think?

I'll have a go at implementing something simple like your code above this afternoon.

[tmeasday]

Ok. I tried it and I realised that my whole idea above was running into the same problem as your original implementation, albeit in a different way.

Here's the issue as succinctly as I can put it (for your sake and my future self's):

I want do something like

var yielder = new Yielder;
currentYielder.withValue(yielder, layoutRenderFunction);

Which sets up the global currentYielder and then runs the layout, which accesses the currentYielder via a {{yield}} helper.

But this suffers a massive problem; which is that layoutRenderFunction could be run again later (because of invalidation) at any time. This means we can't hope that {{yield}} will work properly at this future point, as currentYielder could be anything by then (or most likely it's null).

Obviously making a global yielder that's always there is one solution.

Another solution would be to annotate the HTML of the layout with the yielder and to look it up later when re-rendering. This would require something that I don't know how to do (but don't know if there's a fundamental reason why you couldn't do it): access the current HTML attachment point when running a handlebars helper.

So in {{yield}} I would go "am I in HTML that's being re-rendered?"

• if so, find the closest containing yielders annotation and use that
• if not, use the currentYielder global stack var.

Do you know if it's possible to do this? I'll try and figure it out from the spark source, but you might have some insight.

Note

Of course this is a round about way of doing what we actually want, which is something like (made up Meteor API):

var yielder = new Yielder;
withHelpers(layoutRenderFunction, yielder.helpers);

Where the withHelpers function is a Spark function which augments the set of global helpers with the passed in set of helpers for this rendering function only. If we can solve the problem above we can probably present a succinct solution to this too.

[cmather]

@tmeasday, I finished an implementation that seems to be working well and will push it tonight (once I get it integrated back into evmind). A couple points related to your comment and to problems I ended up solving today:

• Alas, the only LiveRange that will get notified of a re-render is a Landmark. So we could create a new landmark around the layout that responds to re-rendering. But the problem is there is no hook for "I'm about to re-render". You only get notified after its done and on the DOM.
• The design I implemented and seems to be working well for me (but not sure it meets your use case above) has a few components:
  1. YieldPartialDict - Custom reactive dictionary of the form { "yieldName": YieldPartial}
  2. YieldPartial - Has properties for template and data. Also has a render method that combines the two.
  3. yield helper - I was able to isolate this to the router and the given layout template by adding it as a helper to the layout at runtime through the router. The closure allows us to reference the router's YieldPartialDict so it doesn't need to be global.

A couple of changes to the API:

1. waitOn can now be one subscription or an array of subscriptions
2. waitOn and data can be defined at the RouteController level, or passed as options to the render method
3. This allows us to have different behavior for each render call.
4. The default run method will render the main template and then all the to templates as defined in the renderTemplates property of RouteController. You can of course override this in an action or the run method itself.

For example:

HomeController = RouteController.extend({
  loadingTemplate: 'loading',

  notFoundTemplate: 'notFound',

  data: function () {
    return Session.get('content');
  },

  // used automatically if you don't specify an action or override the run method
  renderTemplates: {
    aside: { to: 'aside', data: false },
    comments: { to: 'comments', waitOn: CommentsSubscription, data: {/*data goes here*/}}
  },

  // or...
  run: function () {
    this.render(); // uses the instance waitOn, data, template, etc.

    this.render('aside', {to: 'aside', data: false}); // override data

    this.render('comments', {to: 'comments', waitOn: CommentsSubs, data: {}}); // override waitOn and data
  }
});

[cmather]

See the layouts branch.

[tmeasday]

Looks good!

Transitioner seems to be working (more investigations tomorrow). The key line is:

    pane.appendChild(Meteor.render(function() {
      return Router._partials.get().render();
    }));

Which selectively renders the current router main partial to the correct pane (this is run from an autorun with a dep on Router.current()).

One thing that it would be nice to do would be to override the __defaultLayout__ template (and then the transitioner would "just work" at least in simple cases). Do you think that's possible?

[cmather]

Cool. My plan today is to finish writing the tests, plug the server side back in, and we can hopefully call the first version a wrap. After that, next big items if anyone wants to help:

1. Readme
2. <IE10 support (would be nice to just have this work without the user having to think about adding additional packages. Not sure if history.js is the write way to go, or to just do it ourselves. I'll try to dig into mdn and see what all the idiosyncrasies are.

@tmeasday, On the defaultLayout... is your goal to provide the user a layout that they will not override? Or is it to just set up a different default layout? If the latter, you could set ClientRouter.prototype.defaultLayout.

[cmather]

Okay the latest is on the dev branch. I ran into a snag on writing tests for client_router.js. Having an issue with flushing order. I don't seem to be making any progress so we're going to continue integrating into eventedmind, and bash away at it. Then hopefully the issue in the tests will come to me. Or @tmeasday, maybe we can do this on google hangouts this week.

Keep in mind you need to run your app from a meteor checkout with the latest dev branch. I don't think the linker stuff has been merged into master yet. Also, I disabled the server side for now. Haven't had a chance to add it back using the new WebApp global.

[tmeasday]

@cmather

1. Yes, setting ClientRouter.prototype.layout did the trick, thanks, that's awesome.
2. I wanted to bring over some of my client side tests too;
   • one thing I'm not seeing how to do with IR is route filtering (e.g. access control). How do we achieve this?
   • happy to take a look at flushing problems if you can demonstrate.
3. Re: IE<10 support: I integrated this via a polyfill that simulated window.history. Some notes:
   • The polyfill uses URL hashes, which is a pretty big caveat to know about as a dev (i.e. you need to be careful about using URLs with hashes in them everywhere, in case you trigger it).
   • For this reason (and also I wasn't sure about performance implications) I wasn't comfortable with forcing it to be used, but instead just supported it if it was there. (i.e users have to explicitly turn it on via mrt add HTML5-History-API).
   • Finally you might need to change some event handlers to properly support lower version of IE. This might take some messing about, but shouldn't be too hard.
4. Re: Server. Do you need the server right now? I would suggest getting a version w/ very limited server support first, then once we are sure on the API, it should be reasonably quick to add it. I've noticed for instance that some of the hook code needs to be factored out into lib/router.js.
   • I'll add tests from my old router that we can work to get passing once we agree on the API.
5. README -- Made a start on this. I'll work through it tomorrow, checking test coverage as I go?

I'll be around tomorrow, although I might be out in the morning, so it'll have to be a chat in the evening for you probably. Otherwise later in the week is fine.

[cmather]

@tmeasday,

1. Awesome.
2. Bringing over client side tests sounds great. I'll explain filters and callbacks below.
3. I used the same solution for mini-pages, following your lead. I wonder if there's a way to be smarter about it. I guess this would require conditional package loading which I saw you requested of Meteor some time ago.
4. I agree. Just need to get the basic functionality working again (i.e. linker support, __bootstrap_meteor is not visible anymore). Shouldn't take more than 30 min or so - assuming some unforeseen issues with linker.
5. Very helpful. I see the PR and will come back to it this afternoon.

Can chat tonight if you're around.

Whoops, almost forgot the filter part:

In controller or route definition or global router config:

Controller = RouteController.extend({
  before: /* function or array of functions. reactive by default. reactive: false option turns off reactivity */
  after: /* same as above */

  onBeforeRun: function () { } // before the action is run the first time (not reactive)
  onAfterRun: function () { } // after the action is run (note the templates are not guaranteed to be in the DOM at this time)

  onBeforeRerun: function () { } // before route is run again
  onAfterRerun: function () { } // after route is run again
});

[tmeasday]

So would I do something like:

Router.configure({
  before: function() {
    if (this.name() /* needs access control */ ) {
      this.redirect('login');
    }
  }
})

Is there a way to do it without redirecting? Can we do a soft redirect (i.e. render a different route, without a URL change).

I like to do access control without changing the URL, reactively (if that makes sense)..

[cmather]

There are two options: 1) Create an action or override the run method; 2) Create a before filter.

Option 1:

MyController = RouteController.extend({
  myAction: function () {
    if (this.name())
      this.render('goodTemplate');
    else
      this.render('anotherTemplate');
  }
});

Option 2 would be for the redirect. But note there is no redirect any more on controllers. Just use Router.go.

In case anyone is interested - redirect actually had a purpose in mini-pages: it stopped the page context so pagejs wouldn't call pushState at the end.

But in this new implementation, the Router is smart about that - using computations. Router.current() deps only get invalidated on the last dispatched route (even if multiple are dispatched in the middle of the run).

Basically, redirect works without needing a special redirect method. Also, the previous computation is torn down properly.

[tmeasday]

So is there some way to do 2) without redirecting?

[cmather]

I'm guessing you want to provide a global authorization filter that renders a template but doesn't change the url? Currently I haven't implemented a way to stop the controller from running. Maybe we should add in a stop method? Then if the controller is stopped, the run method and after filters won't execute.

[tmeasday]

Stop the controller from running yes, and also render the correct things.

[cmather]

There's nothing stopping you from calling the 'render' method from within the before filter since the thisArg is the controller instance. But what's missing is a stop method that the router then uses to decide whether to run the rest of the route. It should probably be reset on subsequent runs. If you want to implement it I'll pull it in. Otherwise I can get to it tomorrow - working on our deployment scripts at the moment.

Something like (not looking at the source so I might have missed a few steps):

runController: function (controller, action) {
  controller.stop(false);

  // should we also be able to stop in the onBeforeRun hook and onBeforeRerun hooks?

  controller.runHooks('before');

  if (controller.isStopped()) return;

  // rest of it goes here.
}

[tmeasday]

Oh, right, I thought this was the context. I'll take a look a bit later on.

[tmeasday]

Ok, one final problem I've run into as I'm integrating this.

Determining "page changes" for transitioning.

So, the transitioner is kind of delicate. It kind of breaks the declarative mould of Meteor and can be a bit of a pain consequently. Here's an issue I have:

I need to know when I need to transition. Originally I had code that was something like:

Deps.autorun(function() {
  if (oldContext.path != Router.current().path)
    this.transition();
});

Now, that works fine in transitioning whenever the URL changes. But it's more subtle than that. There are definitely times you want to transition due to filters / waiting etc. Some examples:

1. In my filter example above, I want to transition when the user logs in (or out). [1]
2. In general, I want to transition from the loading page to the real page when the data is ready.

Another thing I tried was just transitioning whenever Router._partials.get() changed, but that means you transition whenever the data context of the template changes, which is definitely not what you want.

I think basically I want to detect whenever the controller calls onRender, although I'm not 100% clear on that.

Solutions

I hope I've explained myself OK and I'm very keen to hear your thoughts.

In lieu of suggestions from you, here are some solutions that might work, in order of how much sense I feel they make:

1. Set YieldPartial.data to a function rather than an object, so that if the value of that function changes Router._partials.get() doesn't invalidate.
2. Put a hook into the router so I know when onRender is called.
3. Set YieldPartial.template to the template name so I can do something like if (oldPartial.template != newPartial.template) (this is always false right now for some reason I don't properly understand).

[1] I could work around this by changing the URL to /login and remembering the old one, although I prefer to leave the URL unchanged when logging in.

[cmather]

@tmeasday,

I have to take a look at your transitioner code to better figure out how we can adjust the router hooks to work with it. I'll try to do that either later tonight, or tomorrow once I get the evmind stuff unblocked by the inheritance work. In the mean time, here are my thoughts:

• It seems like there are two natural relationships between a transition and the router.
• The first is for the entire rendered router; namely the layout and named yields for a router. So the transition region would be the entire thing.
• The second is for a particular named yield inside of a router (or the main yield). If the latter, it seems this is something that would happen at the time of calling this.render. Maybe the API is something like this.render('templateName', { transition: true}).

Walking through this stream of conscious:

Right now the relationships are:

• Router -> layout (rendered router) -> 1 or more named yields -> templates

The named yields get re-rendered when the Router._partials.set('yieldName') gets called, invalidating the computation in the Handlebars yield helper, and causing the new template to be rendered.

So if we're working at the individual template level (a controller calls onRendered which tells the router to call _partials.set), seems like we'd need to clone the existing DOM area and paste it into the transition region, before rendering the new template into the region. One approach would be to create an annotation around that area so you can store away the liverange for a yield. Then on re-render you could see if there's a liverange associated with the named yield and if so, clone its dom nodes before calling set. Need to think more on it.

[tmeasday]

Quick thoughts:

1. Yes, the default layout that the transitioner installs is to do the whole body.
2. I've been doing just the main yield but there's no reason that {{> transitionerPanes}} couldn't take an argument somehow.
3. I've been think about it more at the layout level, i.e. you'll see that rather than {{yield}}-ing my layout puts down essentially a "transitioned" {{yield}} (a.k.a. {{> transitionerPanes}}).
4. I'm not too worried about the mechanism for doing the transition -- what I have now seems to work well. It's more just knowing when a re-rendering should be in-place, and when it should be transitioned.

I think a general rule that works is:

Transition IFF: The template changed OR the URL changed.

Not that "the template changed" is not === "the yield changed" as the yield changes when the data changes too.

[tmeasday]

An additional point: It probably makes sense to give the user the decision at the point of transition. Something like:

Transitioner.transitionType(function(from, to) {
  if (from.templateName === 'loading') {
    return 'fadeout';
  } else if (to.templateName === 'signin') {
    return false;
  }
});

Where false would indicate not to transition, and a string would be a classname to help drive a CSS transition. My experience is an app's transition rules can be complex, and dependant on both where you are coming from and going to.

But a good default (as I outlined above) would be invaluable.

[mizzao]

I've been catching up on your discussion and while I don't understand all of it, I have a question about the implementation of the {{yield}}s.

When using Tom's previous router, the {{renderPage}} was responsible for anything to do with routes, and the rest of the page (perhaps the 'layout') was more or less static. This meant that those parts of the DOM would not get redrawn when routes got pushed around, and this was efficient.

With the yield approach, it seems that all of the yields need to get rendered into whenever the route changes - even if they were to contain exactly the same content as before. This seems like it could be a big performance hit when the sidebar or whatever contains a lot of data that is rendered dynamically.

Is this true or have I misunderstood the implementation?

[cmather]

Hi @mizzao,

I don't think there's a handlebars helper for it yet (PR welcome), but you could achieve the same result as Meteor.Router (tom's first router) by rendering the router into a specific location. It would look like this:

<body>
  <div>Some content</div>
  <div id="router-layout">{{renderRouter}}</div>
</body>

Handlebars.registerHelper('renderRouter', function () {
  return new Handlebars.SafeString(Router.render());
});

In IronRouter, each of the regions is in fact cleared on each route run. An alternative approach would have been to only clear if the user said explicitly to do so. We concluded that the majority of time people would get confused if a previous template was still rendered on a new route. And people would forget to call this.clear() or this.clear('name').

[tmeasday]

@cmather just for the record I realised that I can just check if the template has changed above, so I was able to implement the policy I outlined above. Still love to hear your thoughts if you have any.

[cmather]

Oh Sorry Tom I prematurely closed the issue. I'll reopen or start a new one for transitioner support. Also, will give thoughts on transitioner this weekend.