Dispatcher doesn't save params for previous route

[YAndrew91]

Closely related to the closed #494.

Currently we can't get params a previous route has been executed with. Controllers actions receive route.previous of the following format:

route = {path, @action, @controller, @name, query}

I see, that it can be easily fixed in Dispatcher::controllerLoaded. Makes sense?

[mehcode]

Do you have a use case for this?

The action used to get previousControllerName in the options object and that was about it for previous information.

If added; we'd need to make previous be its own object like previous = {params, route, options} to show the whole thing.

[YAndrew91]

My use case is rather complex: making pop-ups with their own urls. I've described it in my posts in #495. Earlier I have been using my Composer::retrieveStaleComposition for determining 'background' site state, but it is not flexible at all (furthermore, all my solution turned to be quite bad). Now we have route.previous passed in each controller action, so this can solve some part of my issue.

[mehcode]

We'll I'm not against it. @paulmillr @molily ?

To be clear, this would require altering signatures slightly:

class Controller extends Chaplin.Controller
  action: (params, route, previous, options) -> 
    previous #=> {params, route[, options?]}

[paulmillr]

i’m concerned about increasing complexity. Isnt there any other good way of dealing with this thing?

[mehcode]

It wouldn't be too much more added complexity. We're already saving the previousParams inside the dispatcher; it'd be trivial to forward them to actions.

Now that I think about it I can see the use cases here. Perhaps a smart breadcrumb bar or something. It's job could be made a lot easier if it could just push these previous objects into a stack.

[molily]

I don’t like that a controller gets information about the navigation flow and its context automatically. This should be passed explicitly using the Composer. The fact that we save the currentParams at the Dispatcher is a hack to determine whether to start the new controller.

I’m unsure about what’s the best approach here. @YAndrew91, how would previous.params help with your popup problem? Which information do you want to transfer?

My approach would be to create a context composition, retrieve it and re-compose if it exists. This is already possible by using Controller#compose or talking to the Composer directly using !composer:retrieve / !composer:compose. In a composition, you can save arbitrary information.

What’s the tricky part here? Using @chrisabrams’s example: albums/:albumId/:photoId would point to photo#show that retrieves and composes album. This composition has a Collection and a CollectionView, for example. Redirecting back to albums/:albumId is possible because the Collection object has an ID property.

What am I missing?

[YAndrew91]

What’s the tricky part here? Using @chrisabrams’s example: albums/:albumId/:photoId would point to photo#show that retrieves and composes album. This composition has a Collection and a CollectionView, for example. Redirecting back to albums/:albumId is possible because the Collection object has an ID property.

What am I missing?

Looks similar to what I have implemented as the very first solution: you need smth like Composer::retrieveStaleComposition to do it, because !composer:retrieve won't give you a stale composition.

Problem with such approach is that it is not flexible, because you need to write an if branch for each type of 'background' composition, while I want to have my popup seen over the most part of my site states by the same url.

I've already reworked my solution to another approach: Controller::partialTo method, with the same signature as the redirectTo has. So, in my PopupController I create its view&model (actually, I compose its ViewController =) ) and then call this.partialTo(previousUrl).

Currently I get the previousUrl mentioned above from route.previous.path. I'm also filtering here some routes by their names, when I don't want to use them as a background. In addition I set a defaultUrl for the first page load when we have no route.previous available.

So, it would be also great to have this.partialToRoute(route.previous.name, route.previous.params), but I have no route.previous.params for it (also applies to route.previous.options).

PS. I had to noticeably rewrite the Dispather to make this approach work, but so far I'm almost satisfied with this solution.

[molily]

Thanks for the explanation, now I understand the situation better. I understand that a PopupController should be independent and context-agnostic, it should not have code to re-compose all possible contexts. I will play around with Composer/Composition to check if this is possible with Chaplin’s current features.

Thanks for your feedback so far!

[molily]

@mehcode @paulmillr Is there a reason why !composer:retrieve only returns non-stale compositions? Should this only be used on already @composed() compositions? I understand this to some extend. How would you implement the case above?

[molily]

The solution I found looks like this, see my fork, branch composer-example: AlbumsController and ImagesController

The ImagesController is the “overlay” that just reuses the image-context composition without being able to recompose it. That would mean to duplicate code – the ImagseController should not know about all those contexts.

My trick here is to set options: { accepted: true } and to check for options.accepted in the composition’s check method in AlbumsController. If the next controller just accepts the composition by passing these options, it’s marked as fresh. If the ImagesController points back to AlbumsController, the accepted flag isn’t present but the album ID from params is the same, so the composition is marked as fresh. So I can switch between the controllers safely while the album model & view persist.

If there’s only AlbumsController and ImagesController, it’s easier to solve. The composition could be named album and the options would be { albumId: … }. In the example above I’m assuming there are other controllers that link to ImagesController and want to show images in the popup. Also this example doesn’t address special URLs that are derived from the previous controller path (the ImagesController always uses the /images/:id path, no context info in it).

I agree that this is some kind of hack and Chaplin should probably make this easier.

[YAndrew91]

@molily Thanks for the feedback and investigation!

At first sight, I see at least 3 issues in your approach:

• First page load by /images/:id - context will be blank. In my implementation I specify default context for each popup (url or a route name & route params).
• Again, your AlbumsController composes image-context, so it knows the fact that popup can be shown.

In the example above I’m assuming there are other controllers that link to ImagesController and want to show images in the popup.

So, I don't want my ordinary controllers to know anything about popups =)

• The main issue. My context states usually consist of many compositions made by different controllers (hierarchy of controllers and beforeActions). So, I need to recompose all of them to prevent deletion.

[paulmillr]

just wanted to say I have now exactly the same problem. I need to display a lightbox on top of something. I want to implement it with query string tho, like feed, photos/1, feed?page=photos/1. when clicking inside the lightbox on internal link, it should be opened in lightbox too. lightbox then can be closed and user will be brought to a content under lightbox.

I just recorded a video of how my implementation behave (draft): http://files.paulmillr.com/0c3e2R3R0O1U (22sec, 5MB). Notice URLs

@YAndrew91 could you please show partialTo?

[YAndrew91]

I just recorded a video of how my implementation behave (draft): http://files.paulmillr.com/0c3e2R3R0O1U (22sec, 5MB). Notice URLs

Yeah. And my project works with popups very similar to http://www.usatoday.com/ (without context states in urls) Anyway, my solution suits well for your case too. Unfortunately, I can't capture video from my site for now, because it is a business secret =)

@YAndrew91 could you please show partialTo?

Yes, of course. I'll do it as soon as possible, but not sure that I'll have time today, I'm far from my computer now.

[YAndrew91]

@paulmillr , done: https://github.com/YAndrew91/chaplin-popups

Sorry, I hadn't time for rewriting this in coffee (I'm writing in js). Also, the work is still in progress, but I'm already using it in a real application with a lot of different popups.

[molily]

So, I don't want my ordinary controllers to know anything about popups =) (…) My context states usually consist of many compositions made by different controllers (hierarchy of controllers …)

Chaplin’s Composer is a mutual agreement between controllers. Controller A needs to know what it’s giving and controller B needs to know what it’s receiving. This is a very clean concept for a very limited scope, and it fits well into Chaplin’s concept of routes, controllers and strict disposal

There are cases in which B doesn’t need to know how the composition is created. That’s the problem I worked around with my accepted: true hack. I believe something like this baked into Chaplin will solve a lot of those popup use cases. Typically there is just one modal dialog that needs to change the URL. On usatoday.com and moviepilot.com for example, there is no real hierarchy, just one background context and an article in a modal dialog on top.

Typically there is no need to hold the views for more than two screens in memory/in the DOM. Probably it makes sense to hold the models in memory, but than there should be a central cache and a dispose logic that is not tied to controllers, because controllers are volatile.

Chaplin isn’t made for nesting controllers at the moment (we had that discussion in #465), and I consider something like partialTo to be half-baked. It tries to implement something on top of Chaplin which doesn’t fit into Chaplin at the moment. Implementing proper MVC in Chaplin, not just Rails-“MVC” is possible, but will blow up Chaplin’s complexity in the first place. We would need to redefine basically every building stone in Chaplin apps (routes, controllers, model and view relations). There are a lot of good reasons to evolve Chaplin significantly, but IMHO it’s overkill to do so just to solve modal dialogs with their own URLs. For now we should look for an easy solution that fits into Chaplin’s current concept.

[YAndrew91]

Thanks for the feedback first of all! It seems to me that there is a little misunderstanding between us: by 'hierarchy' I don't mean nested controllers, this is a wrong word in this case, but I mean inheritance and beforeActions. So, when opening a popup, I want to recompose all the following things:

• site header
• sidebar
• subheader
• opened tab inside

And all these compositions are done by, say, my tabs controller, which inherits subheader controller and so on... So, yes, there is just one background context.

I consider something like partialTo to be half-baked. It tries to implement something on top of Chaplin which doesn’t fit into Chaplin at the moment. Implementing proper MVC in Chaplin, not just Rails-“MVC” is possible, but will blow up Chaplin’s complexity in the first place. We would need to redefine basically every building stone in Chaplin apps (routes, controllers, model and view relations).

In my example plugin above I have changed only some dispather methods and added the fourth parameter to be passed into actions.

PS. In my partialTo I also implemented an ability to make subsequent (nested) partialTo calls. Yes, it is needed in very rare cases, but I just have such case (popup over another popup) in my project. So, this is an optional feature and can be easily deleted, it will shorten my implementation significantly.

[knuton]

It seems to me that there is a little misunderstanding between us: by 'hierarchy' I don't mean nested controllers, this is a wrong word in this case, but I mean inheritance and beforeActions. So, when opening a popup, I want to recompose all the following things:

• site header
• sidebar
• subheader
• opened tab inside

And all these compositions are done by, say, my tabs controller, which inherits subheader controller and so on...

@YAndrew91 This still sounds like you are sharing code for the coordination of site elements through inheritance, which I think is the wrong approach (see my comment in #495). Again, I think this kind of code is better shared through a Composition subclass placed in a separate module that can be used where appropriate.

Or was this not part of what you meant when referring here? That is, are you not doing this anymore?

(The latter question is partially independent of the popups discussion for now.)

[knuton]

I haven't implemented this, but it seems to me there is an obvious (though unsatisfying) solution to the problem of showing popups (lightboxes & friends) without making "background" controllers aware of them, under the assumption that the presence of popup content is consistently recognisable by path:

Factor out the meat of all background controllers into Composition subclasses, then make the controller that is responsible for popup content use the correct background composition.

Clearly this is unsatisfactory, since you don't want to factor out into separate modules just for that. Then maybe the strategy could be reversed: The router could pass a popup composition along to the background router. This is, I believe, quite similar to what partialTo is already doing.

Q: Is there a good way of making controllers automatically compose composition objects they are passed?

[YAndrew91]

Again, I think this kind of code is better shared through a Composition subclass placed in a separate module that can be used where appropriate.

Can you provide an example for better understanding your idea? :-)

So, for simplicity, now I have a base page controller, which composes only header and footer. Then I inherit my body controller from this base one, use super in beforeAction (and, maybe, ordinary actions), and finally call new Controller::composes, unique for this type of controller. In its turn, some specific body controller can have, say, tabs at the bottom. Then my third type of controllers inherits second one and adds this additional feature.

This is, I believe, quite similar to what partialTo is already doing.

partialTo idea now simply allows Dispather to preserve two (or even more) controllers at a particular time. So, popup/:id URL is mapped to some popup controller which creates popup view and, mainly, calls a partialTo to some context route (this route is usually determined from previous route params, passed into popup's action). In another words, partialTo is like redirectTo, but it doesn't destroy previous controller.

Q: Is there a good way of making controllers automatically compose composition objects they are passed?

+1, I also think how to integrate Controller and Composer more tightly

[knuton]

Can you provide an example for better understanding your idea? :-)

Sure.

I wrote a simple example of code sharing via inheritance. There is a HeaderController which composes and HeaderView, and a HeaderAndFooterController which inherits from it and additionally composes a FooterView. The HelloWorldController then inherits from it to do its specific thing.

I then factored out the HeaderView and FooterView compositions into a HeaderComposition and FooterComposition, removed HeaderController and HeaderAndFooterController, and composed the composition classes in HelloWorldController directly. In this way, shared code can be combined succinctly, yet way more flexibly. You can see the result of the refactoring here.

NB: In this case, since the compositions consist of only one view each, it would be even easier to just require and compose those directly. But clearly the compositions could be much more complex and still be combined in the same way. Since they are classes, inheritance among compositions is also possible where sensible.

[knuton]

@YAndrew91, did you ever take a look at this?