data removing before routing causes rerendering

[sclausen]

If you change the route, the last data is removed first which causes a template rerendering. In my example, if no burgers or pizze exist, the page tells you.
This doesn't happen without iron-router-progress. The bug described in the branch iron-router_bug doesn't occur here!

https://github.com/Phosphoros/iron-router-bug

[Multiply]

I'm not sure if it's a iron-router-progress bug or some weird behaviour when using global hooks with loading templates.

@cmather It seems that the loading template is 'skipped' when using iron-router-progress, even tho, the route isn't done loading yet? Before the shark/blaze-integration branch, I used to inject my code into all routes' hooks instead of the global ones, because of bugs like this. I might be doing something wrong, tho?

[Multiply]

Are you still experiencing this?

[sclausen]

yepp

[tmeasday]

Hey. I think the issue you are seeing here is the same as https://github.com/DiscoverMeteor/Microscope/issues/87

Here's a simple repro https://github.com/tmeasday/IRP22

The basic issue is that if you are going to force the old page to stay around while routing is going on (by calling pause() without rendering), you need to kill reactivity in it. Something like what we do here: https://github.com/percolatestudio/iron-transitioner/blob/f2fb6b516ed609405c9ef80f55a5f7f7e645e9dc/lib/utils.js#L4 (I wish blaze had support for this BTW).

The symptom here is that IR sets the data context to null when it starts running a route, and so blaze will re-render the first page with the null data context. But in general there would be a lot of other subtle problems that could happen if the first page remains reactive while subs are being stopped / data is disappearing etc.

[Multiply]

Ah, I see the problem. I am indeed pausing so it doesn't render the next layout without data.

I am unsure as to how your function is supposed to be implemented here?

[tmeasday]

Actually I just realised something worse -- IR should really clear the current region off the screen if you pause without rendering to it -- the fact that it didn't was a bug that was recently fixed: https://github.com/EventedMind/iron-router/issues/692

I just confirmed that indeed with my repro and IR-devel, A gets cleared off the screen during the loading time.

Hrmmm. This is all a bit of a head-scratcher. Should IR support what you are doing? I think it makes sense that you can't leave rendered templates for old routes on screen when new routes are rendered for the reasons outlined above (subs/data changes etc).

[tmeasday]

cc @cmather

[Multiply]

Effectively I think of IRP as the "loading screen" actually leaving the old screen in tact. Allowing the users of the package, to extend upon that.

It possibly should also allow for showing the loading screen, including the progress-bar?

[Multiply]

I guess if it allowed for the old rendered page to stay, even when data is unsubscribed, it would make it easier to implement transitions?

[tmeasday]

I mean IR could do one of two things if you don't clear a rendered region IMO:

1. Leave it on the screen but kill reactivity in it.
2. Remove it from the screen.

The problem with 1. is it's very surprising. Perhaps IR should have this.render(region, {stop: true}) to achieve this use case or something.

/me is pondering if that would make IT easier to implement.

[Multiply]

I guess the easiest way for me to make it work, is to add something exactly like you suggested, so I could call it to stop rendering completely, then pause and tick the progress-bar until the new route is ready, and when it is, remove the old template, replacing it with the new?

[cmather]

Hey @sclausen, On the first issue mentioned at the top of this thread: I think that issue may be fixed in devel and the next-gen iron router which is on the refactor branch. But even in devel (to be released as soon as Meteor releases 0.8.3) I removed all the sub computations and just created one. This might fix that issue. In the refactor branch (the big iron-router refactor) there is no more getData, setData. Instead, the data function is just passed directly onto the view. So when we stop the outer computation everything should just stop. Can you try it out on devel, and on refactor if you're feeling adventurous?

@Multiply, Trying to understand the thread. Can someone summarize? Is the idea here that you want to keep the old page (showing a loading indicator) until the new page is completely ready to be on the page? I need to think this through. This kind of feels like a high cost to benefit. Is there a reason you can't just load the new page and show the progress indicator until the data is ready?

For Transitioner, it probably requires creating a custom layout that actually manages two sub-layouts.

[cmather]

@tmeasday, in the refactor branch I actually need to give this some more thought. Because I just realized that in the new work, there is no concept of resetting the global data context. It just exists, like a singleton, that can be changed by a route but doesn't have to be. This may or may not be what we want. But I think it's what we want.

Another way to look at it: If we had no router and just a layout (iron-layout) we could do something like this:

var myLayout = new Iron.Layout;
myLayout.insert({el: document.body});
myLayout.template('MyAppLayout');
myLayout.data({tite: 'Some data title'});

So theres only one layout on the page, and its template and data context can change. To change the data context of an individual region, you can override the data like this:

myLayout.render('MyFooter', {to: 'footerRegion', data: {title: 'Footer title'});

So the router just has one layout that gets plugged directly into a controller vs. creating a new Layout on every run. This is to avoid flicker if nothing has changed.

In the old world, we could call the data function (creating a dependency) and "set" the global data context by calling setData(data) on the layout. But now, we just pass the data function on to the layout and it gets called if/when the template actually uses it. I think this should resolve the data rerun issue. But it does leave the question of: if a new controller doesn't do anything with the layout data it will not be cleared; it will be whatever was set last.

[tmeasday]

@cmather I think this is not a good idea. I think we are asking for a variety of horrible bugs if we don't make routes "self-contained" (i.e. if things like data, templates, etc from previous routes stay around if you don't override them).

Let's be super clear here. So the problem in this is issue is that, as you surmised:

1. IRP wants to leave the old template (from a route A) on the page while waiting for a second route B. This is the effect that they are going for.
2. Right now IR sets data back to null as soon as it starts loading B (because data is null while a route is waiting).
3. That means we are at route B, with the subscriptions + data for route B, but the template for route A.

Now, imagine that we don't change the data until B is ready. So now the template + data from A has bled over to B's dispatch.

On the surface it seems great. But think about the way templates are written -- in A's helpers you assume that whatever subscriptions / session variables / etc that are setup by route A are available. You might have code that accesses Router.current() in helpers, which expects it to be A. Plus other things I haven't thought of.

So now you introduce a whole class of difficult to understand errors where in certain circumstances, A's helpers start blowing up and it's not clear why. I've had these kind of problems before, and I can tell you they aren't fun.

[tmeasday]

The one thing we could do (as I've suggested elsewhere), is allow templates to stick around but without reactivity. That way they are just "dead" html and can't trigger bugs :)

[cmather]

Thanks Tom.

The problem with complete self containment is we get flicker. To get complete self containment we would take the entire layout off the page (data contexts, templates and all) and start over on every run. But what we actually do is to say, if nothing has changed from stateA to stateB just don't do anything. In the case of regions we do this by checking whether a new route did not render a region and if not just auto clear it. And if the same template is rendered into the same region again, nothing happens because Spacebars will see its the same value and not re-render.

We could do this with data too but it requires calling the data function in the route run (thereby potentially creating a computation dependency before we really need to) vs. letting the template call the data function when its ready. In the new version of things, since we don't call a data function in the run, and the old computation is stopped before a new one begins, old templates should never run with the new data context.

I think the thing to do here is move the data contexts conversation to actual code in the refactor branch. We can throw out concrete ideas about what to do with real code examples. Here are the relevant lines and the commit where we can leave comments:

• source: https://github.com/EventedMind/iron-router/blob/refactor/lib/route_controller_client.js#L101
• commit: https://github.com/EventedMind/iron-router/commit/6866776e450a4a5b50191956ca09d072b583b2f0#diff-b2d0347006b0defab274161f2c101369R101

[tmeasday]

At a high level: that makes sense to me. I think we need to make sure at the router level that weirdness can't happen though. Let's work through it.

PS As an FYI for this particular issue @cmather, it may have slipped your attention but it's kind of moot as IRP only works because of a bug in IR (see my above comment https://github.com/Multiply/iron-router-progress/issues/22#issuecomment-47314117). So there's that to consider.

[cmather]

Yeah, we should definitely work through it :). It's possible I missed something or there's a better approach!

Also, related to the second point about the bug. The router only has one layout by default. But, if flicker wasn't really a concern, I can imagine another scenario where you DO take the layout off the page and put another one, or the same one, back on the page. For example, maybe you create a completely new layout on every run. And the previous layout you can do what you want with (like move it from one side of the page to another, etc). There isn't currently a stop() method on dynamic template, but we could investigate adding one (I think meteor Views support something like this now).

EDIT: You have access to the underlying view and range of the layout easily: layout.view and layout.range.

[tmeasday]

Right. So I think the conclusion of this conversation is that they way to achieve IRP is using Iron Transitioner, which will allow the concept of keeping the old screen on the page.

This seems like a good conclusion to me.

PS it'd be nice if blaze could avoid flicker in the case when you render a second layout which ends up rendering the same HTML. I won't pretend to understand the complexities of HTML patching that make that difficult though.

[Multiply]

I guess some people don't want the old page to linger around, and just show their loading template including the progress bar, but sure, if the old page has to linger around, iron-transitioner seems like a good way to deal with this issue.

[tmeasday]

Hey @Multiply

1. If they want to show a loading page with nprogress on top, they don't really need a plugin for it, they can just call out to nprogress in the rendered and destroyed callbacks of their loading template.
2. If they want to show the old page, they need some kind of transition/animation plugin. I created a proof-of-concept way of doing this here: https://github.com/percolatestudio/iron-transitioner/blob/packaging/nprogress/nprogress.html
   • basically it does exactly the same thing, except that it leaves the old page on the screen while the loading page is visible.
   • Right now it's using a POC Iron Transitioner {{#transitioner}} helper to do that, but actually it can be done in a more generic way I think, using an animation helper such as the ones provided by momentum.
3. I'm thinking we'll probably end up doing something simpler for Microscope, but if you want to work through the method outlined in 2., I'm happy to help. Let me know.

[Multiply]

Both solutions above, doesn't work exactly as intended, for this package, I think. At least it doesn't support ticking whenever a subscription is ready.

We need to support two ways of showing the progress bar:

1. Stay on the old template, show a bar till we're ready with the new route.
2. Show the loading template, while also showing the bar.

Both solutions seems to favor iron-transitioner, so I guess I have to work it in there.

I'll ponder a bit about the different solutions, till I get home later today.

[tmeasday]

Hey @Multiply - I think I'm maybe missing something, or I'm just confused. For your case 2., could IRP not just be as simple as:

Router.onBeforeAction(function(){
  if(this.ready()) {
    // show nprogress
  } else {
    // hide nprogress
  }
});

Am I missing something? Case 1. requires more effort though, for sure.

[Multiply]

I don't know if my latest changes has addressed this issue, but I think not.

IRP now should support the case of showing the loading template, or showing an empty page with just the progress-bar. It doesn't support staying on the old page, and first render the new one, when it's ready. I'll look into how we handle that.

Edit: I might make a weak dependency for iron:transitioner and use it, if it's available. If it is, it'll stay on the page, show the progress, and first render when it's ready? If there's no transitioner, we assume it has to either show an empty page while loading, or show the loading template.

[tmeasday]

@Multiply I think the issue is addressed because as you say you aren't leaving the old page on the screen any more. As for implementing that feature, perhaps a separate discussion is warranted.

[Multiply]

Closing this issue, due to IR and IRP being refactored. If this still happens with the newest versions of both, please reopen, with a new reproduction. :)

@tmeasday Let's open a new discussion for implementing transitions as well.