marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.06k stars 1.26k forks source link

rethink marionette router #1177

Closed samccone closed 10 years ago

samccone commented 10 years ago

Currently the Marionette Router is lacking in multiple ways

For marionette to continue to grow this is going to be a critical problem that we need to solve.

Take a peak at this https://github.com/angular-ui/ui-router for a great example of what a router should do.

Previous Issues:

jamiebuilds commented 10 years ago

Addressing the concept of state, I really like when query parameters handle this.

/things/2?foo=true
/:view/:model?state

My favorite implementation of this that currently exists is in ember. I like how the query parameters belong to the controller (which would be more like the view in the backbone universe).

I'm not sure if that would work for everyone, it could also be part of the router, and broken off into some concept of subroutes.

brian-mann commented 10 years ago

I'm documenting what I consider the de-facto solution to routing, which approaches angular ui's robustness, ease of use, and magic. After its documented I'll release it here for feedback. Sam has seen it when I went up to Providence. He's really jealous.

The Good News

Some of the features:

I call it --- Application Router. That's what Marionette needs. An Application Router class with all of this kind of support.

The Bad News

Right now it's highly coupled to our/my very specific workflow, and this class needs to be much more configurable to more generic needs. I know not everyone uses controllers (you're crazy by the way), or splits up their apps across multiple files, so maybe this is a Marionette as a framework kind of thing, I don't know.

-Brian

jamesplease commented 10 years ago

Before / After hooks. Return false from a before which prevents routing

Is this a triggerMethod thing? If so, this behavior will be inconsistent with every other triggerMethod.

brian-mann commented 10 years ago

No, I was saying you get before / after callbacks to execute arbitrary code prior to the invocation of your controllers, or anything else kicking off.

Returning false would stop everything in motion.

Nothing in the application router uses triggerMethod, it uses a lot of $.Deferred()'s however.

On Tue, Apr 15, 2014 at 11:01 AM, James Smith notifications@github.comwrote:

Before / After hooks. Return false from a before which prevents routing

Is this a triggerMethod thing? If so, this behavior will be inconsistent with every other triggerMethod.

Reply to this email directly or view it on GitHubhttps://github.com/marionettejs/backbone.marionette/issues/1177#issuecomment-40492134 .

jamesplease commented 10 years ago

ok, awesome :)

StevenLangbroek commented 10 years ago

That sounds awesome @brian-mann!

chiplay commented 10 years ago

FWIW we use this sub-app router lib, which is pretty similar to @isochronous version that we forked. It sounds like @brian-mann has the solution though.

I'd love to see a routing config that we could somehow share between the client and server as well - for parsing routes in Node.

isochronous commented 10 years ago

Hey, Chip!

Glad to hear something I did helped you out :). I really should create a real repo for that thing, and not just a gist. Looks like I should definitely incorporate the additions you made - that immediate triggering of sub routes is a case I never had to handle, but it's a common enough use case that I should have designed for it from the start.

On Apr 19, 2014, at 3:33 PM, Chip Lay notifications@github.com wrote:

FWIW we use this sub-app router lib, which is pretty similar to @isochronous version that we forked. It sounds like @brian-mann has the solution though.

I'd love to see a routing config that we could somehow share between the client and server as well - for parsing routes in Node.

— Reply to this email directly or view it on GitHub.

isochronous commented 10 years ago

Oh yeah... And you could use a JSON file to provide the routes in a format that would be consumable to both the client and server. Since I'm using requireJS, I'd just use the JSON plugin for it to load the route definitions, but that's definitely not necessary.

On Apr 19, 2014, at 3:33 PM, Chip Lay notifications@github.com wrote:

FWIW we use this sub-app router lib, which is pretty similar to @isochronous version that we forked. It sounds like @brian-mann has the solution though.

I'd love to see a routing config that we could somehow share between the client and server as well - for parsing routes in Node.

— Reply to this email directly or view it on GitHub.

chiplay commented 10 years ago

@isochronous Hey! Long time :)

That has been a great pattern for us - we're using it to lazyload our 'namespaces' or packaged modules using our Require.js build. It's a pretty interesting setup - but working well so far:

Our main router:

return Marionette.AppRouter.extend({
  appRoutes: {
    '': 'index',
    ':namespace(/*subroute)': 'invokeSubRoute'
  }
});

And our main controller (edited for brevity):

var AppController = Marionette.Controller.extend({

  /** @method index */
  index: function() {
    this.invokeSubRoute('home');
  },

  /**
   * @method invokeSubRoute
   * Takes all routes for the system and lazyloads
   * the namespaced modules via require
   * catch-all route = ':namespace(/*subroute)'
   * @param {String} namespace
   * @param {String} subroute
   */
  invokeSubRoute: function(route, subroute) {
    // eg. /product/{id}/review/{id}
    // namespace = 'product'
    // subroute = '{id}/review/{id}'
    var _this = this;
    route = route || 'home';
    route = route.toLowerCase();
    if (subroute) subroute = subroute.toLowerCase();

    if (!_.contains(app.namespaces, route)) {
      // 404 Logic, etc
      var msg = 'Route is not a defined namespace';
      throw new Error(msg);
    }

    // Stop current running modules
    _.each(app.namespaces, function (ns) {
      if (app[ns]) app[ns].stop();
    });

    _this._startModule(route);
  },

  _startModule: function(route) {
    var deferred = q.defer();
    require(['ns/' + route + '/' + route],
      function () {
        app[route].start();
        deferred.resolve();
      }
    );
    return deferred.promise;
  }
});

var appController = new AppController();

Then each namespace has its own router:

app.module(namespace, {
  moduleClass: BaseModule.extend({
    routes: {
      '': 'index',
      'cart': 'cart',
      'history': 'history',
      ':id': 'showOrderById'
    },
    ...
  })
});

and finally in the "BaseModule" class:

addDefaultInitializer: function () {
  this.addInitializer(function (options) {
    var Router = Marionette.SubAppRouter.extend({
      appRoutes: this.routes
    });

    this.router = new Router(this.moduleName, {
      controller: this.controller,
      createTrailingSlashRoutes: true
    });
  });
}
isochronous commented 10 years ago

I gotta say, that's a pretty clever pattern. Looks like you've gotten a lot more comfortable with Backbone :)

On Apr 19, 2014, at 3:56 PM, Chip Lay notifications@github.com wrote:

@isochronous Hey! Long time :)

That has been a great pattern for us - we're using it to lazyload our 'namespaces' or packaged modules using our Require.js build. It's a pretty interesting setup - but working well so far:

Our main router:

return Marionette.AppRouter.extend({ appRoutes: { '': 'index', ':namespace(/*subroute)': 'invokeSubRoute' } }); And our main controller (edited for brevity):

var AppController = Marionette.Controller.extend({

/* @method index / index: function() { this.invokeSubRoute('home'); },

/**

  • @method invokeSubRoute
  • Takes all routes for the system and lazyloads
  • the namespaced modules via require
  • catch-all route = ':namespace(/*subroute)'
  • @param {String} namespace
  • @param {String} subroute */ invokeSubRoute: function(route, subroute) { // eg. /product/{id}/review/{id} // namespace = 'product' // subroute = '{id}/review/{id}' var _this = this; route = route || 'home'; route = route.toLowerCase(); if (subroute) subroute = subroute.toLowerCase();

    if (!_.contains(app.namespaces, route)) { // 404 Logic, etc var msg = 'Route is not a defined namespace'; throw new Error(msg); }

    // Stop current running modules _.each(app.namespaces, function (ns) { if (app[ns]) app[ns].stop(); });

    _this._startModule(route); },

    _startModule: function(route) { var deferred = q.defer(); require(['ns/' + route + '/' + route], function () { app[route].start(); deferred.resolve(); } ); return deferred.promise; } });

var appController = new AppController(); Then each namespace has its own router:

app.module(namespace, { moduleClass: BaseModule.extend({ routes: { '': 'index', 'cart': 'cart', 'history': 'history', ':id': 'showOrderById' }, ... }) }); and finally in the "BaseModule" class:

addDefaultInitializer: function () { this.addInitializer(function (options) { var Router = Marionette.SubAppRouter.extend({ appRoutes: this.routes });

this.router = new Router(this.moduleName, {
  controller: this.controller,
  createTrailingSlashRoutes: true
});

}); } — Reply to this email directly or view it on GitHub.

chiplay commented 10 years ago

@isochronous just a little bit ;) we should catchup sometime and grab a beer. Ever at any of the ATL meetups?

jamesplease commented 10 years ago

@brian-mann any updates? I'd like to see what you've come up with.

brian-mann commented 10 years ago

I'll gist you guys this week with all the details.

On Thu, May 8, 2014 at 2:31 PM, James Smith notifications@github.comwrote:

@brian-mann https://github.com/brian-mann any updates? I'd like to see what you've come up with.

— Reply to this email directly or view it on GitHubhttps://github.com/marionettejs/backbone.marionette/issues/1177#issuecomment-42587209 .

jamesplease commented 10 years ago

I've only been using the router for 2 weeks now or something, but my idea for a really, really simple, yet still powerful implementation can be seen in this gist.

Basically just Express routers brought over to Marionette

jamesplease commented 10 years ago

A working example of the gist I made above can be seen here.

It's just a rough draft, though, for the purpose of discussion :)

@isochronous I replied on the gist. I'd like to hear more of your thoughts.

JSteunou commented 10 years ago

I'll be interest with having "name" for route. So we can call a route from controller or print in template without knowing the URL behind, and also be able to change the URL without making a CTRL+F on all source project to update the URLs.

jamesplease commented 10 years ago

:+1: @JSteunou I'm interested in those ideas, too.

samccone commented 10 years ago

Yep @JSteunou see angular UI's state based routing

StevenLangbroek commented 10 years ago

Or EmberJS's router:

App.Router.map(function() {
  this.resource('posts', { path: 'blog' });
});
JSteunou commented 10 years ago

Not liking the EmberJS declaration way but the idea is here.

StevenLangbroek commented 10 years ago

No i'm not a fan of the declaration either, I'm also a big fan of having multiple routers (which Ember doesn't allow as far as I'm aware), but having a path separate from the name of the route makes alot of sense to me:

var BlogRouter = Marionette.AppRouter.extend({
    appRoutes: {
        'posts': {
            path: 'blog',
            action: 'listPosts'
        },
        'post': {
            path: 'blog/article/:post_id',
            action: 'showPost'
        }
    }
});

var blogRouter = new BlogRouter({ controller: someAwesomeBlogController });
var blogPost = App.request('blog:entity:latest');
blogRouter.transitionTo('post', blogPost.id);
jamiebuilds commented 10 years ago

I would definitely like to see some form of formal transitioning mechanism. ie. transitionTo and even template helpers like linkTo

cmaher commented 10 years ago

Regarding transitionTo, I think it makes sense for it to be (at least optionally) event-based by default.

var MyRouter =  Marionette.AppRouter.extend({
  controller: myController,

  appRoutes: {
    'posts/:id': {
      action: 'showPost',
      event: 'myPost'  // used by transitionTo
    },

    'posts': 'myPosts'  // works as normal
  }

  // implement to override default behavior
  // onTransitionToMyPost: function: (id) { ... }
});

var router = new MyRouter({ 
  vent: App.vent, 
  reqres: App.reqres
});

// navigates to /posts/1
// calls myController.showPost as usual
// does not trigger 'route'
App.vent.trigger('transitionTo:myPost', 1); 

// gets a link to /posts/1
var post1Href = App.request('linkTo:myPost', 1); 

Pros

Cons

isochronous commented 10 years ago

The only problem with that is that it adds a hard dependency on the EventAggregator, which thus far has been avoided.

On Thu, Jun 19, 2014 at 11:43 PM, Christian Maher notifications@github.com wrote:

Regarding transitionTo, I think it makes sense for it to be (at least optionally) event-based by default.

var MyRouter = Marionette.AppRouter.extend({ controller: myController,

appRoutes: { 'posts/:id': { action: 'showPost', event: 'myPost' // used by transitionTo },

'posts': 'myPosts'  // works as normal

}

// implement to override default behavior // onTransitionToMyPost: function: (id) { ... }}); var router = new MyRouter({ vent: App.vent, reqres: App.reqres}); // navigates to /posts/1// calls myController.showPost as usual// does not trigger 'route'App.vent.trigger('transitionTo:myPost', 1); // gets a link to /posts/1var post1Href = App.request('linkTo:myPost', 1);

Pros

  • Backwards-compatible
  • Reduces boilerplate
  • Decouples navigation from router instance

Cons

  • 'event' must be unique across all routers on same vent (can be somewhat alleviated by adding a namespace)

— Reply to this email directly or view it on GitHub https://github.com/marionettejs/backbone.marionette/issues/1177#issuecomment-46642300 .

jamesplease commented 10 years ago

@isochronous I don't think avoiding the event aggregator is a goal that we should have. It's mere coincidence that it hasn't been used yet in core...perhaps because of the very fact that our architecture Classes are so weak. As it happens, Wreqr – or rather it's successor, Backbone.Radio – will be merged back into Marionette core when the architecture update rolls around. And if utilizing any of the features of Backbone.Radio can make for more powerful architecture then we should definitely take advantage of that.

I'd like to clarify that I'm not defending @cmaher's specific implementation. I haven't really thought about it too deeply yet. But I'm fine with leveraging the Event Aggregator, or any other part of Wreqr, if it makes things better.

isochronous commented 10 years ago

The only reason I bring that up is that Marionette bills itself as a composite application architecture, which means there shouldn't be too much inter-dependence between its modules. Don't get me wrong, I love the EventAggregator and use it heavily, but using it to power the router just feels a little weird to me. I like the fact that that approach would allow you to trigger several actions for any matched route, but the angular-style router Brian Mann proposed would do the same thing, with the advantage of enforcing a certain order to the callbacks. Doing the same thing in an event-based way would require very careful manipulation of the callback stack for a given event. And if you really want an event-based router, you can already do that quite easily with the existing code by leveraging the onRoute method:

onRoute: function (name, path, args) {
    // not sure what the difference between name and path is, so you might
    // want to use name here instead of path
    vent.trigger('route:' + path, args);
}
cmaher commented 10 years ago

@isochronous Triggering an event on a route is exactly what I want to avoid. See http://lostechies.com/derickbailey/2011/08/28/dont-execute-a-backbone-js-route-handler-from-your-code/ and the routing section in the sample of https://leanpub.com/marionette-gentle-introduction. Relying on route events from Backbone.Router to control program flow (instead of just reflecting it) can lead to painful workarounds.

My proposal doesn't require events to be used; it just enables them. router.transitionTo('myPost', 1) as @StevenLangbroek proposed would still work.

jamiebuilds commented 10 years ago

I doubt using events to power the router would be necessary. this.transitionTo would just be a helper function really that delegates directly to the routing system and possibly assists in some sort of cleanup.

isochronous commented 10 years ago

Ah, okay, I misunderstood

On Fri, Jun 20, 2014 at 1:18 PM, Christian Maher notifications@github.com wrote:

@isochronous https://github.com/isochronous Triggering an event on a route is exactly what I want to avoid. See http://lostechies.com/derickbailey/2011/08/28/dont-execute-a-backbone-js-route-handler-from-your-code/ and the routing section in the sample of https://leanpub.com/marionette-gentle-introduction. Relying on route events from Backbone.Router to control program flow (instead of just reflecting it) can lead to painful workarounds.

My proposal doesn't require events to be used; it just enables them. router.transitionTo('myPost', 1) as @StevenLangbroek https://github.com/StevenLangbroek proposed would still work.

— Reply to this email directly or view it on GitHub https://github.com/marionettejs/backbone.marionette/issues/1177#issuecomment-46703517 .

jamesplease commented 10 years ago

For our consideration: this library and the planned 0.2.0 API

samccone commented 10 years ago

issue last updated in 2012

JSteunou commented 10 years ago

@jmeas I very like the filters features from this routerManager. This one + named routes feature like this lib or this one + multi-router that can be started / stopped along with modules / sub-app would be top notch

ahumphreys87 commented 10 years ago

@jmeas I think im still in favour of some automagic linking between the router and a new Controller - i think we've discussed over on that issue but the idea was that a route of account/profile would call the profile method on the accountController. Then we have the option of passing args into this account/profile/$1 would pass $1 into the profile method.

I think this is more along the lines of a traditional MVC router and a format a lot of people will be familiar with. It also gives Marionette a 'real' controller. Something like this would be great for SPA's

JSteunou commented 10 years ago

@ahumphreys87 this impose to much for those not having the same structure you want to force. What about allowing multiple routers and let people inject the controller to the router as it is now, with extra options to inject a bunch of controllers and associate routes with a specific controller.

This will allow SPA with one router one controller as now. One router with multiple controllers for those who like having <resource>Controller. But also multiple routers each one having its controller for big apps structured with multiple subapps / modules.

paulovieira commented 10 years ago

Another related plugin: https://github.com/brentertz/backbone-route-control

StevenLangbroek commented 10 years ago

ping @brian-mann: any word on your implementation? will begin scaling up the app I'm working on soon, and want to investigate good routing solutions.

samccone commented 10 years ago

@StevenLangbroek @marionettejs/marionette-core is working on this beast :+1:

Lots of ideas have been flying around hopefully we will have some api specs to show soon :)

samccone commented 10 years ago

Migrated to the v3 list https://docs.google.com/document/d/1Sx1YE2SJg-NGSGsd8mELf3wpywI-UyOvCkufbm6klOw/edit#

newtonianb commented 9 years ago

ping @brian-mann message from Apr 15, 2014, that all sounds incredible! is it ready to show?

urosgruber commented 8 years ago

@brian-mann any news on this gistm even if it's just WIP.