Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design #1

Closed
ProLoser opened this issue Jan 18, 2013 · 126 comments
Closed

Design #1

ProLoser opened this issue Jan 18, 2013 · 126 comments

Comments

@vincentdieltiens
Copy link

First of all, I'm happy to try coding with you guys a better routing system :)

I suggest that each participant sees the 2 other solutions (Like @ProLoser said)

I think that the first step, before implementation, is to determine what we want or need. In other words we have to set the specifications of this project.

So this is what i think is important :

  • a system that makes the relation between one route (url) with one or more views or controllers.
  • a system that supports nested views
  • a system that can reload views only if necessary. By this I mean that:
    • lets the route #/resources list the resources
    • lets the route #/resources/:resourceId list the resources and show the selected resource
    • if the user go directly (via browser location bar) to #/resources/42, it must load the list of resources and the resource 42
    • if the user goes from #/resources to /resources/42, it must only load the resource 42 (the list of resources doesn't change)
    • if the user goes from #/resources/42 to #/resources, it can (it must be a choice) not reload the list of resources but it can clear the view containing the resource 42 (or not, i have one case in my project where i don't want to clear some views, i can explain it if you want)
  • a system that doesn't change to much the code of an application that comes from angularJS routing system to our system. By This, i mean, we don't have to change the whole structure of a project if it uses this routing system. Only routes registrations must be changed (not templates/controllers, or not a lot)

From what I can see of other projects, here are few notices :

  • In the solution of @hkdobrev :
    • there is no support of control of what can be (re)loaded or cleared.
    • if the user goes directly using the location bar, it loads only one view ! That is the main reason why i didn't use his solution before implementing mine
  • In the solution of @bennadel :
    • as he says, it is for simple logic websites not for complex systems.

Here are some pros that my solution offers (by this, I do not mean that it's perfect) :

  • The solution handles well nested views (actually in my project at work, i've 4 levels... and it's not the most complicated part of the project)
  • The developper can determine when a view have to be reloaded or cleared. (But it's not always easy to understand)
  • The solution proposes only to change the directive ngView by multiView and the $route provider of AngularJS by the multiRouteProvider. That means that that whole structure of the project does not have to be changed completely.

Well, i'm excited about your point of view ;-)

Vincent

@lrlopez
Copy link

lrlopez commented Jan 19, 2013

@vincentdieltiens Cool, I didn't know about your proposal.

I'd like to add a summary and some additional comments about both @bennadel and @vincentdieltiens approaches: (@hkdobrev proposal lacks "selective" reloading, which I think is critical for any multiview system).

  • About @bennadel solution:
    • The key of this proposal is using an "action path"
      • An action path is a colon delimited sequence of values (i.e. "dashboard.clients.detail").
      • Each possible route is matched to an action path.
      • Each of the values in an action path is associated to a controller (and optionally any number of route params) which is responsible for rendering the content (i.e. "/show/:customerID" path matches "dashboard.clients.detail" with param "customerID").
      • Different controllers can share an action path prefix (i.e. "dashboard" and "dashboard.clients"). This behavior allows easy implementation of nested views.
      • Each controller only reacts to changes to its prefix in the action path, and also in the relevant params. This allows partial refreshing as the view only gets updated if the param changes (i.e. customerID) or the view appears/disappears from the URL.
    • Pros/cons
      • Pro: It's easy to understand how it works (his blog entry clarifies it even further).
      • Pro: Action path easies hierarchical subview design.
      • Con: It's rather difficult to implement a subview (you need a lot of "skeleton" code).
      • Con: A subview cannot be reused easily (i.e. using the same view for "dashboard.clients.detail" and "dashboard.provider.detail" even if the show the same kind of data).
  • About @vincentdieltiens solution: (please correct me if I'm wrong)
    • His proposal is based on defining multiple controllers in every route
      • Each controller is associated to a subview (and its template) and has access to all the route params.
      • When a route changes, each controller gets questioned twice:
        • Do you need to be run again? This allows the subview to check if some kind of update is needed on itself.
        • Do you want to be kept in memory? This allows the subview and its controller to be persistant even if it gets out of scope.
    • Pros/cons
      • Pro: It's also easy to understand how the inner code works.
      • Pro: Creating a subview is simple.
      • Pro: It allows reusing subviews/controllers in different routes (i.e. same code to show details for clients and providers).
      • Con: It's not integrated directly into the Angular native routing system. But it is a minor one because it works as a drop-in replacement. Don't get me wrong, but this could lead to missing some routing enhancements/fixes that get pushed into the main project (i.e. angular/angular.js@c639261 and angular/angular.js@faf02f0 )
      • Con: Some controller parameters have misleading names (i.e. "execute" function... may something similar to "updateNeeded" could be a better name)

Good work Ben & Vicent :)

@vincentdieltiens
Copy link

@lrlopez Thanks. My solution is rather new. There is also a topic about this : https://groups.google.com/forum/#!msg/angular/ayG1hCUOfX0/eVxb7fNqRgQJ

I need to read more in-deph the solution @bennadel . I will try to do this quickly :)

@lrlopez : you are right about my solution. When i developped it, i didn't find good names (I often lacking of vocabulary in English).

I didn't have proposed my solution to the angularjs team because :

  • I wanted to test it correctly before. Also i did'nt have time (and because I didn't have the fortitude...) to write test. The angularjs team don't accept enhanced that not come with test.
  • There are somes parts that could be enhanced. One example is storing some informations (oldRouteInfo and persist) using the data() function of jqLite/jQuery.

So, you are right, I have to merge my solution with the enhancements/fixes that are applied to the angularjs Routing system.
But, if we made a really good solution we can also pull-request it ... :)

There is also one Pro of my solution : it works with the syntax of the angularJS routing system. So no breaking.

@ProLoser
Copy link
Member Author

I didn't understand the @bennadel solution at first. I was getting confused between the routes and action paths, the action paths in no way resemble the routes. Creating a second layer of abstraction people must learn and fundamentally understand.

My vote is @vincentdieltiens approach (overall) with some modifications. An ideal world I aim to use the core routing system. However I realize this may not be enough.

On @vincentdieltiens's solution:

  • It should be 1 module, not two
  • All options should degrade to expected behavior gracefully and should be minimalist:
    • Passing a string rather than an array to controllers to specify 1 controller
    • Having 1 view without a name (so it can be inferred as app-wide)
    • Resolve support (might be done by execute or persist?)
    • Generating the ID from the route string + controller?

My main concern is intuitive usage

@ProLoser
Copy link
Member Author

I also like @jeme suggestion of using sub-routes. It would be fairly trivial to add recursive appending:

.when('/article', {
  controller: 'ArticleList',
  templateUrl: 'articles/index.html',
  subroutes: {
    '/:articleId[/:tab]' : { // -> '/article/:articleId[/:tab]' (the :tab token is optional, the :articleId is mandatory)
      controller: 'ArticleDetails',
      templateUrl: 'articles/details.html'
    },
    '/categories' : { // -> '/article/categories'
      controller: 'ArticleCategoryList',
      templateUrl: 'articles/categories/index.html',
      subroutes: {
        '/:categoryId' : { // -> '/article/categories/:categoryId'
          controller: 'ArticleCategoryDetails',
          templateUrl: 'articles/categories/details.html',
        }
      }
    }
  }
})

Actually after writing that ^^ I find that syntax fucking smexy. AND super intuitive!

@bennadel
Copy link

Few key hurdles trying to allow each view partial to remain as independent as I could; and, to allow each view partial to respond only to relevant changes. We have need for three different, distinct layouts and several layers of nesting.

@vincentdieltiens
Copy link

For the two solutions proposed by @ProLoser :

  • The first one: For small projects, it's a cool way of writing things. But what if you have (and that's my case) :
    • A list of customers (/customers)
    • Where a customer has general informations tab (/customers/:customerId)
    • A tab with contacts in this company (/customers/:customerId/contacts) (with a subroutes for /customers/:customerId/contacts/:contactId)
    • A tab for prices deaded with this customer (/customer/:customerId/prices)
    • And other tabs ?

How do you deal with /customers/:customerId/contacts and /customers/:customerId/prices ?
In other words, this proposal doesn't support multiples subviews correctly.

  • The second one: This is far better than the first one. It is sexy but there is one constraint (correct me if i'm wrong) : you have to define your route with subroutes in the same module. I explain. In my project (sorry for always using it, but it's my main real example ;-)), I seperate my controllers, for readability and maintenability, in differents modules : customer, customer.contact, customer.prices, customer.statistics, and so on. I like to define routes that are related to the contact module in the contact module. It's easier to manage and to maintain. But contact routes are subroutes of the customer routes...

That's why, in my solution, I store "super" controllers (controllers that are used for several routes), in global variables that can be accessed in different modules. And It's not a good way (i prefer to avoid global variables).

It will be great to find a solution with the proposition of @jeme where we can "append" subroutes.
Maybe something like :

$routeProvider.when('/article', {
  controller: 'ArticleList',
  templateUrl: 'articles/index.html',
  subroutes: {
     ...
  }
});

$routeProvider.addSubroutes('/article', '/article/:id/coments', {
  controller: 'ArticleCommentList',
  templateUrl: 'articles/comment/index.html',
  subroutes: {
    ...
  }
});

Maybe an other idea ?

@lrlopez
Copy link

lrlopez commented Jan 20, 2013

@bennadel, welcome!

I also like @jeme proposal of subroutes. They successfully degrade to the AngularJS traditional routing if subroutes are not used. This means that is backward compatible so allows older code and examples to continue working even if the new routing model gets pulled into the main codebase. They are also cleaner to understand and to implement. So, 👍

About @ProLoser idea of optional parameters, may be now is the right time to propose enhancements to the route specification. Some weeks ago I had a look into the different proposals, and one of the most flexible was @xealot's angular/angular.js#1634, which uses the werkzeug routing syntax.

That new syntax would allow to specify the typed parameters (integer only, path which includes slashes, etc). I.e. "/client/int:bar" or "/search/path:where/string:what".

The new implementation also remembers the generated regexp for each route in order to reduce the overhead, so it should be even faster than the original code.

We could use @xealot work as a base and expand the syntax to include optional params (which I find extremely useful) or, if is interesting enough, we could fully implement werkzeug route spec (i.e. minlength, maxlength, maps, etc.)

Of course there are other alternatives that are worth a look (first section).

What do you think?

@lrlopez
Copy link

lrlopez commented Jan 20, 2013

@vincentdieltiens,

I think the case of /customers/:customerId/contacts and /customers/:customerId/prices would be covered by something similar to this:

.when('/customers/:customerId', {
  controller: 'CustomerCtrl',
  templateUrl: 'customers/index.html',
  subroutes: {
    '/contacts' : {
      controller: 'CustomerContactsDetailCtrl',
      templateUrl: 'contacts/details.html'
    },
    '/prices' : {
      controller: 'CustomerPricesDetailCtrl',
      templateUrl: 'prices/details.html'
    }
  }
})

What I miss is another parameter that specifies which subview should the template of each subroute render in. Without it, you are implying that each view can one have one subview inside, and that may not be ok on some cases.

Take a look into this:
MultipleViews

You'd need to specify in which view you'd like to render each template of every subroute (the main route could always be rendered on the top one, ngView). This would also be backward compatible with the current routing implementation.

.when('/customers', {
  controller: 'CustomerManagementCtrl',
  templateUrl: 'customers/index.html',
  subroutes: {
    '/manage' : {
      controller: 'CustomerListCtrl',
      templateUrl: 'customers/list.html',
      subview: 'CustomerListView'
    },
    '/:customerId' : {
      controller: 'CustomerDetailCtrl',
      templateUrl: 'customers/detail.html',
      subview: 'CustomerDetailView'
    }
  }
})

Also, dynamically adding subroutes would be nice, but I'm concerned that it could break the relation between one route and its relevant subviews. Why? Because if the browser reloads the page, it could lead to a different set of routing rules if you're not careful enough.

@vincentdieltiens
Copy link

@lrlopez

I had understood the idea. But that's does not allow to register subroutes in several modules and that's a pity.

@jeme
Copy link
Contributor

jeme commented Jan 20, 2013

Appreciated that people like the idea of sub-routes, basically I ended up defining the "interface" as i wanted it before I started implementing the solution. Thats where I often start anyways... If things are to complex or obscure they pose to big a barrier in my experience, and then they get no traction...

I might question a little if this is something we can add to Angular-UI alone, currently I have core changes to route.js. This might be a change that ultimately won't be difficult to get into the Angular stream though as it is isolated and should be backwards compatible... Currently I have had more of a prototyping approach to it, so haven't worked much with testing etc...

Anyways... Within the router I have changed the parseRoute to the following (which in long term should be the only needed change to route):

/**
 * @returns the current active route, by matching it against the URL
 */
function parseRoute() {
    // Match a route
    var params, match;
    forEach(routes, function (route, path) {
        if (!match && (match = matchRoute(route, route, path, $location.path()))) {
            return match;
        }
    });
    // No route matched; fallback to "otherwise" route
    return match || routes[null] && inherit(routes[null], { params: {}, pathParams: {} });
}

/**
 * @returns the root route if it or any of it's decendants are a match.
 */
function matchRoute(root, route, fullPath, currentPath) {
    var params, match;
    if (!match && (params = matcher(currentPath, fullPath))) {
        match = inherit(root, {
            params: extend({}, $location.search(), params),
            pathParams: params
        });
        match.$route = root;
        if (route != root) {
            match.$subroute = route;
        }
    }

    if (route.subroutes) {
        forEach(route.subroutes, function (subroute, subpath) {
            if (!match) {
                match = matchRoute(root, subroute, fullPath + subpath, currentPath);
            }
        });
    }
    return match;
}

You can also see my fork here: https://github.com/jeme/angular.js

Still getting into the way they structure their code, so fighting allot with what closures there is etc.
The above works in my sample atm. though... (and I can see now I have forgotten to delete params in the parseRoute method it self... Oh well >.<)

@vincentdieltiens
Copy link

@jeme : with your syntax, how will you determine :

  • if a subview must be cleared or not ?
  • if a controller (of a subview) must be executed or not ?

@jeme
Copy link
Contributor

jeme commented Jan 20, 2013

@vincentdieltiens currently it is left to the controller since I don't have a directive implemented yet that supports sub-routing, but the idea is that you go down the ladder until you hit something that is changed, from there you execute the views... (that also means that I as of now don't have sub controllers, what @ProLoser typed was an extented version of what I had)

This will require that we know the actual route-chain post route update, that is not added to the above yet as i have to do things very incrementally right now, simply because I am not that familiar with the original source, this also boils down to a requirement that ones the routing part is done it should be fully backwards compatible...

One of the things I would like to provide however is a flexibility where you might choose to execute an entire route if it's parameters has changed, or in the controller react to that change... As often when parameters change, we wan't to use the same template, yet populate it with different data, so re-executing that route in that case also seems "un-needed" to me...

Instead the controller could just react to the route, load the data and update the scope... I have yet to find a design for that though, where it is optional behavior.

@vincentdieltiens
Copy link

As often when parameters change, we wan't to use the same template, yet populate it with different data, so re-executing that route in that case also seems "un-needed" to me...

Instead the controller could just react to the route, load the data and update the scope... I have yet to find a design for that though, where it is optional behavior.

I don't get it. By executing the route you mean executing the controllers of that route ?
For me, if the route changes from /resources/42 to /resources/43, the template that corresponding to the view showing the resource informations don't change but the data change, so we have to re-execute the controller with the new parameters to re-populate the view. I think it's not the job of the controller to know if it must re-populate the view or not. It's the job the framework.

A controller get a route and his parameters, then it get the data via resources (or an other way) and populate the view and maybe do other thing according to the routing parameters. If the parameters change, it must be re-executed.
And i think that it's far easier to implement (and easier, for the developper that using this routing system, to understand how it works).

But that's my point of view, maybe you can convince me that i'm wrong ;-)

@lrlopez, @ProLoser and @bennadel : what do you thing about it ?

@bennadel
Copy link

Sorry guys, getting into this conversation later than I would have hoped. I haven't had a chance to review the code samples yet, but just a few thoughts.

As you what @vincentdieltiens was saying:

I think it's not the job of the controller to know if it must re-populate the view or not. It the job the framework.

I think this is one of the ways in which traditionally server-side and client-side frameworks start to split paths. On a server-side framework, in which the entire response is re-built for every request, it is the job of the framework to determine which Controller should load data... and when.

On a client-side application, however, I think there is far too much persisted UI for this same model to hold true. Now that the UI structures are far more complex and now that we are refreshing only portions of a page, I think the "when do I refresh" must fall to the responsibility of the Controller. After all, a Controller may choose to load a sub-view that isn't tied to a specific route, but responds to route changes in general.

As a really contrite example, imagine a view that simply counted the number of pages a user had visited. That View would be tied to route changes, but not in a way that the framework would ever know about.

@jeme
Copy link
Contributor

jeme commented Jan 20, 2013

@vincentdieltiens you might have misunderstood me a little... I certainly agree that what you mention is the default and most common behavior and that it would serve 95%.

Yet for unforeseen cases, I was thinking of providing a way to hook into the framework and "override" the behavior for a specific route, rather than leaving people in a situation where they had to create a whole new directive similar to the one we might provide, since that again would become a barrier to some...

It could be as simple as an event/call back that gets called if registered that returns true or false. The constraint however is that it needs to be close to the controller in my mind, so it's a optional "hook" you attach to either in the controller or the parent controller. (properly the controller it self, that should be ok)

And also, this won't the the routing providers responsibility, that should ALWAYS raise the $routeChangeStart and $routeChangeSuccess on any of the before mentioned cases, this will be something that could be added to the new view directive. So it doesn't pollute in the core.

@bennadel
Copy link

Also, there is definitely a strong possibility that a single view could depend on two different route parameters. I have that in my app in which time-stamps are part of the URL:

/updates/:projectID/:since

... where projectID is an ID, and :since is a UTC milliseconds value. If the URL changed and only the :since value changed, you couldn't simply reload the entire Controller as that would require the project to be reloaded as well. Instead, what you'd want is the controller to say, "The ID is the same, so use the currently loaded project; but, the Since has changed, to reload the updates."

@bennadel
Copy link

@lrlopez, this is definitely an interesting idea:

That new syntax would allow to specify the typed parameters (integer only, path which includes slashes, etc). I.e. "/client/int:bar" or "/search/path:where/string:what".

I had thought of trying to add some sort of validation, using RegEx (I am biased in that I love regex) that would be provided as part of the route definition. Something like:

.when(
    "/projects/:id/:tab",
    {
        action: "splash.home"
    },
    {
        id: /^\d+$/i,
        foo: /(info|contacts|audit)/i,
        tab: /(\w+)/i
    }
)

In this case, the last hash would contain a key-value pair in which the key was the route param and the value was the RegExp instance that would be run (ie. .test( .. )) against the matching parameters.

In the end, I decided not to go with this since I didn't see a huge win in "guarding" my app. After all, the only time a URL would be bad would be if someone was purposefully messing with the routes. And, in that case, either a view wouldn't be rendered (due to no matching ngSwitchWhen case); or, the server would throw some 4xx error since the requested parameters for data were not valid.

I had a number of "validation" parts built into my original framework; but, I ended up stripping most of them out. As long as people use the app in the way it was designed, route-based validation shouldn't be needed. And, for people who abuse the app, I see no need to jump through hoops to help them get good views.

@bennadel
Copy link

On something that @lrlopez said about template reuse:

Con: A subview cannot be reused easily (i.e. using the same view for "dashboard.clients.detail" and "dashboard.provider.detail" even if the show the same kind of data).

I have seen this thought on server-side programming as well - people are always looking for a way to reuse views with different controllers. In my experience, this typically only works in low-design applications. I work with a lot of designers who like to curate and customize every view to its exact use-case. At first, you think you think you can reuse a view in two different parts of your application. But, over time, your UI team / product manager / users want more and more customizations in a given view.

Ultimately, ... and again, this is just my experience ... shared views tend to be broken up over time because they start to have too many conditionals.

That said, you can always have shared views in an application. There's nothing to say that the view:

/views/project/detail.htm

... can't load, as part of its UI:

/views/shared/project-widget.htm

.... that maybe shows up in a number of places.

@jeme
Copy link
Contributor

jeme commented Jan 20, 2013

@bennadel I think the werkzeug syntax would allow for a regex converter, sort of (if implemented to specs):

.when(
    "/projects/<int:id>/<regexp(exp=\w+):tab>/<xstring:bar>
).converters( { regexp: RegExConverter, xstring: XStringConverter } );

Or something like that at least...

http://werkzeug.pocoo.org/docs/routing/

@vincentdieltiens
Copy link

@jeme : I had understood the idea.
@bennadel : Ok. This might be a good example of a pro to handle data reloading in the controller. Even if it can be handled by a route /projects/:projectId/ and a subroute /projects/:projectId/:since (but it's not as clean as handling this in the controller).

I have a feeling that something could not work with this behavior but i can't put a word on it. I will sleep on it, maybe tomorow will be better :D

About typed parameters, I think too that it's an interresting idea but as @bennadel explained it, it's maybe an "overhead" that is not always useful. But it can be an optional feature of the routing system. The way @bennadel would writing it, is like Symfony (in php) do. It's easy to understand and easy to use and can be optional.
I think the werkzeug syntax is far harder to read that the proposal of @bennadel.
To facilitate the progress of this project, I propose to open a new topic on typed parameters and focus here on "how subroutes" will works ;-)

@bennadel
Copy link

@vincentdieltiens you're approach is incredibly clever. I think your understanding of how templates are compiled and linked is much stronger than mine. I don't believe I would have ever come up with a way to manually breakdown and re-link templates and keep all the $scope chain stuff in tact. Very impressive!

That said, I think that our two approaches actually have a good deal in common. Your execute() ( and the related persist() method) seem to be doing what my:

if ( renderContext.isChangeRelevant() ) { ... }

... and

if ( requestContext.haveParamsChanged( [ "categoryID", "petID" ] ) ) { ... }

... are doing. The major difference is that you put your configuration in the route declarations and I put my configuration in the Controllers themselves. Internally, my "Request Context" , stores the "action" and current / previous route params, so it can be queried as to what has changed.

@bennadel
Copy link

@vincentdieltiens , I was thinking about your approach and looking at my sample app, specifically this UI:

2013-01-20_1806

URL: http://bennadel.github.com/AngularJS-Routing/#/pets/dogs/5

This UI has the "Pet Detail" UI. And, it has the nested, "Related Pets" UI (see red boxed). Both of the UI values depend on the change in route:

  • Pet detail selects the given pet based on :petID.
  • Related pets selects a random pet (of the same type) that is NOT the :petID

If I were to change the :petID in the URL, both of these UIs have to update; but, one it nested inside of the other. Would you define both of those in the route provider? Or, would you have the "detail" view be tied to the route handler; and the "related pets" widget simply fall back to using $routeChangeSuccess?

@vincentdieltiens
Copy link

@bennadel Thanks

Yes we have handled the same problems (param changing) but in a different way.

I don't know what's the best solution : defining this in the configuration of the route (like my solution) or in the controllers (like your solution).
All I know is the reason why I put it in the route configuration. I come from a php environment and work since long time ago with Symfony (an MVC framework). A best practice in Symfony is always doing the simpler action/controller you can. This is for readability and maintainability and I totally agree with this.

And more you handle things in controller, greater the controller will be...

When I'm writing a controller my focus is :

  • which data in need and retreive it
  • which actions i need (save(), and so on)

When I'm writing a controller my focus is not :

  • is this controller must be cleared if the route is nore more like this.
  • is this controller must be re-executed if the routing has changed.

In this way, when I read a controller all i need to know is which data it is retreiving and which actions exists.
Because I know that, according to the route configuration, the controller will be cleared/or rexecuted when it's needed.

If I were to change the :petID in the URL, both of these UIs have to update; but, one it nested inside of the other. Would you define both of those in the route provider? Or, would you have the "detail" view be tied to the route handler; and the "related pets" widget simply fall back to using $routeChangeSuccess?

I tend to define it in the via the route provider because of the reason above. Maybe it's the role of an other layer than controller and route provider.

@ProLoser
Copy link
Member Author

God damned it's getting near impossible to follow this thread. PLEASE START POSTING TL;DR

Notes:

  • I think the nested definition is layout is the best
  • Complex route parameters
    • I DON'T see the purpose of complex/typed routes
      • No real added benefit of ints/other
      • A URL is fundamentally a string
    • Optional parameters should be added
  • Controller Reloading
    • If a parent route changes, all descendants reload @bennadel
    • If a child route affects a parent controller, the parameter should be moved up (and made optional?)
    • If child route no longer matches, children are destroyed
    • Defer further edge-cases for post v1.0
  • Define routes in multiple locations
    • Add additional andWhen() method
    • Can we defer this for v1.0
    • Is it easy to specify the parent route?
  • It is correct that the 1 flaw with my original example is selecting the view to update
    • Simply add a view name parameter (such as [details] or [stats]) @lrlopez
    • Names can be reused
    • Names are optional (no-named routes show up in no-named views, useful when working with simple apps)
      • Nesting level becomes a deciding factor when using UNNAMED routes
      • Naming a route overrides the nesting level specifity (useful for floating sidebars/widgets)
  • Core or side project
    • For now lets develop this outside of the core (copy+paste core if necessary or extend if possible)
    • Once more stable, we can propose merger into core

I believe that this design covers every requirement you can throw at it EXCEPT defining routes in multiple places.

@ProLoser
Copy link
Member Author

In an attempt to reach a consensus I started this set of specifications:

https://github.com/angular-ui/router/wiki

Please add your votes or expand specifications.

Do NOT discuss bullets or add complex explanations.

Keep bullets concise.

@bennadel
Copy link

@ProLoser sorry to be long winded :) I'll try to be more bullet-pointy.

@vincentdieltiens I'd be curious in trying to re-write my sample app using your approach. Hopefully I can try that tomorrow night.

@vincentdieltiens
Copy link

@ProLoser sorry too

About my vote :

  • Can append more children later in separate method call: andWhen('/parent/route', '/child')
    -1 because I don't see the purpose of this idea. Why would you do that ? same controller for 2 routes ?
  • Parameters should support typecasting syntax: /user/int:id
    +1 but not with this syntax.
  • View name should be optional If nested view, unnamed route uses unnamed view at the same nesting level.
    -1. Because
    • Harder to read the code later. It's easier to read in the route view: 'myView' and in the template <div ngView="myView"> that beginning to think about at which level this view is and to which route it corresponds
    • Harder to implement. In fact, the view has no idea of which nesting level he is.
    • The only benefit is to write 3 words less. That's not enough for me
  • All descendant controllers are reloaded.
    -1. Why reloaded ? and not cleared / empty / deleted ?
    A example /customers/42/contacts to /customers/43. Parent route changes but the subview displaying contacts must be deleted because subroute a not matching anymore
  • $routeUpdated event is $broadcast instead of reloading controller : Always use event
    -1. As I said before, maybe it's a good idea but i'm not convinced at all. See my previous comment about that
  • If child route no longer matches
    • controller is unloaded
    • related view is emptied

-1 for both. In some case you don't want to unload and clear. Imaginge a "tab panel" with, for each page tab, a named view. If you go from a tab to another, you don't always want to reload the data and repopulate the view because you have great chances to come back to this tab with few chances that data has changed (and in that case, i prefer to use websocket to update the data in real time)

@timkindberg
Copy link
Contributor

Like this? https://github.com/timkindberg/router/blob/master/router_api_playground.html. Should I do a pull request?

I named the file router_api_playground.html because I feel like we need to establish the API first, making it as user friendly as possible, and then try to make it work. We can then change things as needed during implementation if absolutely necessary.

@ksperling
Copy link
Contributor

@timkindberg I've had a look at your API -- I like the ability to define nested states "fluidly" like that, however I think an API like that should be sugar on top of a documented simple "here's a state with these properties" expressed as a plain old object API. I think it's important to

  • be able to add additional child states outside the definition of the parent, so that the entire state machine doesn't have to be defined in a single module
  • be able to reference states via an object/variable rather than just a string name. To this end I've now rewritten the implementation such that $state.current is the exact object passed to $stateProvider.state(). Referencing parent states and transitioning to a state via $state.transition(nameOrState) also allow either a name or the state object to be used, so if desired strong references can be used instead of strings essentially everywhere.
  • be able to integrate with other state machine libraries or otherwise customize the state model.

For these reasons I think it's advantageous if the $stateProvider is somewhat flexible in what exactly a state object looks like, and does not strictly require them to be configured via the high-level configuration "builder"-style interface.

I agree the ability to have the state provide data to the controller is crucial, but I think the existing route.resolve mechanism already covers this, as the functions referenced by route.resolve are injectable and can therefore reference the current $stateParams.

I've done some work on my prototype and split out the URL routing from the actual state transition mechanics, and to treat state objects as outlined above, as well as removing the requirement of hierarchical state names, and allowing URLs of sub-states to be either relative (the default) or absolute (by prefixing them with '^', in which case the sub-state URL will still need to provide all the parameters required by the parent state, but can do so with a different syntax). I'm just working on putting the pieces back together so the sample works again.

@timkindberg
Copy link
Contributor

I like your plan. Having a more generic back end that is flexible and adding a sugar API on top. I am not an angular expert. I'm partaking in this discussion because as I compare frameworks, it's coming down between angular and ember. Routing in ember just seems much more powerful while remaining simple to use. So I may miss the mark a bit with my API ideas but I am useful for serving as the "layman" perspective in this discussion. That's why I place so much emphasis on the API, because that is what will sell this to the simple folk like me. That said, I know you have a deeper knowledge of what needs to happen technically.

I like your bullet points 2 & 3, but wonder why 1 is so necessary. I feel that setting multiple views per state is more important. If we must use an object literal instead of function objects then I guess the object could have a views array where each array item would be an object setting up a single view.

Regardless I look forward to your work. The last version was very good.

@ksperling
Copy link
Contributor

@timkindberg I've just pushed the updated working implementation. The named-view part is still missing, but other than that the internals feel much tidier now than in the first attempt. It seems being backwards-compatible with $route will be reasonably easy as well.

I definitely agree that a good and easy to understand API is important though!

Just trying to wrap my head around how multiple named views should work conceptually, and how to handle cases like

  • A state says to load something into view X, but there is no such view
  • A nested state replaces a view that is also assigned by one of it's ancestors. When that state is entered, what happens to the existing view and controller? The simplest solution would be to destroy the existing controller, and simply re-render the template and create a new controller instance when the nested state is exited, but that could cause state to be lost. But hiding or detaching the existing view and then re-attaching it to the DOM later sounds rather error prone and complicated.

Another area that needs some thought is what happens during a state transition? A transition involves resolving templates and controller dependencies asynchronously, and if this step fails a reasonable expectation would be for the UI state to remain unchanged and an error event to be broadcast. However this means that during that time, $stateParams, $state.current etc all still reflect the previous state. At the moment I'm working around this by overriding $stateParams for $injector.invoke() when resolving dependencies, but I'm not sure if this is the most intuitive approach.

Another related question is what happens if another transition is initiated while one is already in progress -- should the first one be abandoned, or the second attempt refused?

@timkindberg
Copy link
Contributor

Reviewing your code in depth right now. I think you are doing a good job. Let me put on my reviewers hat on if I may.

  • Shouldn't the parent state be automatically detected? Meaning it would just parse the state name and remove the last instance of '.' and following characters? Is there a reason we'd want to explicitly set the parent to be something that doesn't match the state name—e.g. State "a.b.c" has a parent of "x.y"; wouldn't it always have a parent of "a.b"? Maybe you added this because you removed the requirement for hierarchical state names? If so would this be an optional parameter? Also I'd still like to see auto parent state detection if full hierarchy name is given.
  • I've grown to like your non-nested state declarations much more this time around as we'll as the fully qualified state names, "a.b.c". It really helps envision the state's place in the hierarchy.
  • is the run method boilerplate? Or would it be used for other things as well?
  • I'm not sold on the abstract state idea. What is the point of a state you can't transition to? A state should represent a state in the app, so I think we need a way to specify a state's index child state, like in my API. So if transitioning to an abstract state it goes to it's index child state. A state would have a Boolean index property. We may not need the abstract property.
  • I think for concurrent transitions, previous ones should be aborted. Most recent wins.
  • I agree that if a state transition fails it should stay on previous state and broadcast error.
  • I vote implicit ng-state-view levels.

Wow, again your code looks very well written. So on to multiple named views:

  • the directive naming could work like ng-app. E.g. ng-state-view="myNamedView"

  • if view X doesn't exist throw error, let dev deal with it.

  • as for your second question about nested states trying set a view that an ancestor view has set; I don't think this should be possible. States can only set views at their own level, so doing away with abstract states and forcing a child index will fix this. Look at my API again to see how this wouldn't be an issue with a child index state.

  • Lastly, a user friendly way to set multiple named views via the literal object is going to be tough. I liked my approach, so I'm wondering how we could incorporate it into a JSON style. The only thing I can think of is an array of objects like this:

    .state('contacts.detail.item', {
      parent: 'contacts.detail',
      url: '/item/:itemId',
      views: [{
          viewName:"myNamedView",
          templateUrl: 'contacts.detail.item.html',
          controller:
            [        '$scope', '$stateParams',
            function ($scope,   $stateParams) {
                $scope.item = findById($scope.contact.items, $stateParams.itemId);
            }]
       }, {
           ... More View Objects ...
       }]
    })
    

    So yeah it's late and I typed all of this on an iPad.

@ksperling
Copy link
Contributor

@timkindberg: Thanks for your feedback!

I wanted to be able to specify 'parent' via a direct object reference, but I agree putting automatic derivation of the parent via hierarchical names sounds good -- it seems to me this would be used in 95% of the cases. I'll put that back in for the case where 'parent' is undefined (so it can be specified as null to override automatic derivation)

run() is part of the module API, I've just used this in the sample to publish some stuff in the scope rather than using an application-level controller.

I think an abstract state is very similar to an abstract class in an OO language -- so I'm finding it quite intuitive. In the sample 'contacts' is abstract and simply defines the sidebar, but leaving the definition of the right-hand part of the UI to the child states. The contacts state can't be activated without also activating one of it's children, e.g. contacts.list (which you call the "index" state, I'm currently simply using the empty URL relative to the parent state. Note that this only works because the URL of the abstract parent state is not itself added to the routing rules -- otherwise the rules for the parent and the default child would conflict).

I suppose it would be possible to derive the fact that 'contacts' is abstract from the fact that it has a child state with an identical URL, but I think that would make things less obvious -- I think as it stands an abstract state is rather easy to understand: It's a state that can only become active indirectly via one of its sub-states, and it's URL is not added to the router. Note that it also prevents you from doing $state.transitionTo('contacts'); it's not purely tied to URL.

Apart from the use case in 'contacts' for common UI, I think having common services / data can also be a good use case for abstract states (via the 'resolve' property). The locals that are made available to controllers inherit down the state hierarchy, so the values resolved for the (maybe abstract) parent state are also available to sub-state controllers.

I've got the automatic nesting level working now, but still pondering the exact semantics of multipel named views -- you say "States can only set views at their own level", I guess that would be a reasonable restriction and would avoid some of the pathological cases. The states now have optional onEnter / onExit callbacks, so those could maybe be used to do custom 'weird' stuff, by setting a scope variable that's picked up by an ng:include for example.

In terms of JSON syntax I was thinking (within the state object):

  views: {
    main: {
      template: ...,
      controller: ...,
      resolve: ...,
    },
    sidebar: {
      template: ...,
      controller: ...,
      resolve: ...,
    }
  }

Having a 'views' property would mean that the implicit default view properties (template, controller) in the state object itself may not be used (but you can still have a view named ""), however 'resolve' can be meaningfully used both at the state and view level to have shared and/or controller specific data.

It feels like this is getting pretty close to a full working solution!

@timkindberg
Copy link
Contributor

  • auto parent derivation. Awesome.
  • run() method. Ok.
  • abstract. Sill not sold. Please allow me to attempt to convince you one more time:
    • It's a term that is not friendly to the majority of common developers. I do totally get the concept, but I think it's a tad too geeky for common folks. On the other hand the concept of a child index is a common theme seen in many other routing libraries and implementations. I think we should follow that convention to provide new users with a lower learning curve.
    • Really the two concepts are identical behind the scenes. Only difference is you use an abstract property and empty child URL, I propose no abstract property and a "/" child URL.
    • Using the slash for index is instantly recognizable to anyone vaguely familiar with routing; I feel like its also more clear like "ok, this is the child state that will load automatically".
    • I think users are going to assume that they CAN transition to a parent state and have it load the child index.
    • perhaps I'm missing a technical restriction (I admit I didn't understand 100% of your code, though I tried). So I stated my concerns but I trust your final judgement, just keep the simple folk in mind. :)
  • I do love idea of having common services / data inherit down the hierarchy. Awesome. I think this would still function fine with the child index concept ;)
  • automatic nesting. Radical.
  • JSON syntax. Love it!
  • love how state and view levels can both have template and controller. Can I see an example of what resolve is and how it would work, like how would you pass in custom scope info to a controller?

This IS starting to be a perfect solution in my opinion! No else seems to be weighing in, I wonder if we're missing anything. Also just want to applaud you again for doing the actual coding. It's clear that you know what you are doing.

@ksperling
Copy link
Contributor

I've just pushed an updated implementation and sample (also updated the hosted sample at http://filedrop.plukmobile.net/angular-states/sample/index.html) It supports hierarchical names now as well as multi-view.

Multiple named views

What finally made this "click" for me and seems to have resolved all the implementation problems is that view names (by default) need to be taken not as absolute names, but as names relative to the parent state / template.

In the sample, the root template (index.html) has an unnamed ng-state-view tag. The absolute name of this view therefore becomes '<unnamed>@<root>' (which in practice is just '@' because both the unnamed view and the root state have the empty string as their name). The "contacts" state defines a templateUrl for the unnamed view; because the contacts state is a child of the root state, the absolute / resolved name for this view is '<unnamed>@<root>', i.e. the content goes into the view tag just mentioned.

The contacts.html template loaded by the contacts state now contains another unnamed ng-state-view directive. The resolved name of this view is '<unnamed>@contacts', because the view directive has no name attribute (hence unamed) and is within a template loaded by the contacts state. It also contains an ng-state-view="menu" directive. The resolved name for this view is 'menu@contacts'.

The contacts.detail state defines views that reference these two directives in turn: The unnamed view resolves to the absolute view name '<unnamed>@contacts' (i.e. '@contacts'), because contacts.detail is a child of contacts. The view name 'menu' resolves to 'menu@contacts' in just the same way, and both line up neatly with the directives in contacts.html.

The contacts.detail state contains a third view definition for 'hint@'. Because this view name is already absolute (it contains an '@'), it is left untouched and references the ng-state-view="hint" directive in the root template. The contacts.detail.item state then overrides this view with a different template.

So putting it all together, the rules as currently implemented boil down to this

  • A view directive species a relative view name (defaults to the empty string). This name is automatically qualified with the name of the state that loaded the template that the directive is in.
  • A state references the view directive to load content into by name. If the name is relative, it is automatically qualified with the name of the parent of the state being defined (this is because the parent's templates generally contain the directives to be filled by the child states). If the name is absolute it is taken as is.
  • At link time, each view directive simply looks up the absolute view name in the current state, inheriting up the state chain if necessary.

With these rules, both the very common use-case of simply nesting unnamed views that I started with, as well as more complex cases like the global 'hint@' view in the sample work as expected.

One key insight for me into when thinking about schemes for figuring out which view goes into which directive is this: When a view directive is encountered, it needs to be able to figure out it's absolute name without being able to look at any nested templates that will be loaded into it eventually. This means that while a child state can override a view defined by a parent state (e.g. the "hint@" view should now display xyz rather than what the parent state said), it doesn't make sense to want to override where in the DOM a view goes (e.g. view "hint@" should now go here rather than where the parent template had it defined).

The other aspect that seems a little restrictive at first is that when a state references a directive, e.g. 'menu' in the sample, it intuitively makes sense to want to not just blindly qualify this with the name of the parent state (i.e. menu@contacts), but instead walk up the chain of parent states until we find one that does provider a place to put that view. The reason this won't work is that states only provider view placeholders indirectly, via the ng-state-view directives inside the templates that get loaded (and these can even be provided by functions / providers dynamically based on parameters or other runtime state). I think it's important to be able to look at the definition of a state and be able to understand 'statically' what it's view definitions mean, without having to trace through all the templates that could possible loaded by the parent states.

One extension that I think might be OK is to allow an ng-state-view directive to specify an absolute name, but I'm struggling to think of a use case for this.

Next Steps

'abstract'

I hear what you're saying regarding 'abstract' not being a well-understood concept. It's also not a term that's generally used in the context of Hierarchical State Machines. I'll think some more about the idea of designating a 'default' child state (which is a common concept with HSMs), however

  • I don't think this idea should be tightly coupled to URLs (with your index "/" proposal you also seem to treat "/" somehow differently from other URL patterns?)
  • The default / index state cannot have additional parameters on top of what the parent state defines, which is another thing that would need to be validated at state definition time
  • Adding a default state to a parent state after it has been defined effectively changes that parent from being a "concrete" state to an "abstract" state. I would prefer if the meaning of a state could be understood by looking at just the state itself (and it's parents), and not have this meaning be changed radically when somewhere else a child state is added to it that happens to designate itself as the default child.
    • As can be seen from the preceding points, the idea of a "default child" is a lot "heavier" in terms of constraints and implementation than the concept of an "abstract" state -- an 'abstract' state is simply a state that can't be transitioned to (because it's simply there to provide some UI or dependencies that are common to it's child states), and the implementation takes up all of 2 lines of code

resolve / $injector

When trying to demonstrate the use of 'resolve' I noticed that even though I'm setting up a nice inheritance chain of resolved dependencies (with the intention that child state controllers have access to dependencies resolved by parent states) the $injector service explicitly excludes inherited properties from injection (this behaviour isn't documented though). So either

  • we do without this type of inheritance, a controller gets only the dependencies that are defined directly for it, and access to anything else has to go via the $scope (where a parent state controller could put stuff for the child state controller to access).
  • we change the behaviour of $injector. The documentation makes no statements on inherited properties on the 'locals' object either way, so this could be an option.
  • we work around the problem by manually walking through the inheritance chain and flattening out the dependencies objects. I'd rather not do this.

It would be good to get some feedback on what others think would be reasonable here. Having resolved dependencies inherit down the state hierarchy seems very neat, but if it's really needed in practice is debatable. Maybe somebody has used the corresponding 'resolve' feature in $routeProvider and could comment on this?

move into angular-ui repo, docs, tests

And other tidy-up.

@timkindberg
Copy link
Contributor

Wow lots to take in. I'll try to review tonight. I've been very excited waiting for this update.

@timkindberg
Copy link
Contributor

For anyone coming into this now the latest code is here: https://github.com/pluk/angular-extras. Anyone on this mailing list should review the current state of Karsten's implementation and provide feedback/make pull request as needed.

@ProLoser
Copy link
Member Author

ProLoser commented Feb 8, 2013

Are we going to be relocating the project?
On Feb 7, 2013 5:23 PM, "Tim Kindberg" [email protected] wrote:

For anyone coming into this now the latest code is here:
https://github.com/pluk/angular-extras. Anyone on this mailing list
should review the current state of Karsten's implementation and provide
feedback/make pull request as needed.


Reply to this email directly or view it on GitHubhttps://github.com/angular-ui/router/issues/1#issuecomment-13272463.

@timkindberg
Copy link
Contributor

Karsten needs to move his code into a fork or custom branch of this repo. I think he/we should also add lots more comments in the code for easier review.

@timkindberg
Copy link
Contributor

Man I don't have time to write up a thorough review (wish I did) but I did review for the last hour or so and I just want to say that I LOVE IT!! I think you've done an exceptional job here. I feel like you've taken everyone's concerns into consideration and covered all the major requirements (at least from my requirements). I agree with all of your insights and statements about the restrictions you've set. They make sense to me and actually don't feel very restrictive at all.

My opinion on your next steps:

  • I'm fine with abstract, it's grown on me and your points are valid.
  • Resolve: are you saying that only the inheritance feature of resolve does not work? I can still resolve some data on a controller of the same state though right? If so, I think that is enough for now.
  • Docs. I think this is huge! because you have little nuggets and shortcuts in the API everywhere. That's what makes it so great. Also by documenting it we'll see where we need to improve the API to be more user friendly
    • One thing that comes to mind is the "@" in absolute view names. It technically makes sense but I'm thinking i'd rather always see something on both sides of the "@". So instead of "hint@" I think I'd like some identifier like "hint@root". It doesn't look as good after writing it though, and it could cause problems if there is state called root. I could just see the lone "@" being another thing in angular that people have to refer to docs to understand (feels a bit too 1337 to me)
  • We need more commenting in the code (hate to say it but this may be all on you Karsten, you've done so much already). There are some very heavy regexp and logic parts in there. If anyone wants to actually help with pull requests this will certainly help (unless you enjoy doing all the work :) ).

@timkindberg
Copy link
Contributor

I just realized one thing I don't like about the new views API is I'd prefer that each view object represent the view, but right now the view name is not part of the view object, its the key. It'll be easier to explain with code:

Currently it is:

views: {
    'viewName': {
        templateUrl: 'contacts.detail.html',              
        controller: controllerFn...
    },
    ... more views ...
}

But I kind of like how vincent's approach worked with arrays of view objects, I'll just show you:

views: [
    {
        view: "viewName",
        templateUrl: 'contacts.detail.html',              
        controller: controllerFn...
    },
    ... more views ...
]

The reason I like the more is the view objects could be created elsewhere and it would be clear as part of the object which view it was going to plug into. What do you think?

@ksperling
Copy link
Contributor

Hm I guess that makes sense too. It would be easy enough to support either an array with explicitly named views or an object with views named by key.

Tim Kindberg [email protected] wrote:

I just realized one thing I don't like about the new views API is I'd prefer that each view object represent the view, but right now the view name is not part of the view object, its the key. It'll be easier to explain with code:

Currently it is:

views: {
   'viewName': {
       templateUrl: 'contacts.detail.html',              
       controller: controllerFn...
   },
   ... more views ...
}

But I kind of like how vincent's approach worked with arrays of view objects, I'll just show you:

views: [
   {
       view: "viewName",
       templateUrl: 'contacts.detail.html',              
       controller: controllerFn...
   },
   ... more views ...
]

The reason I like the more is the view objects could be created elsewhere and it would be clear as part of the object which view it was going to plug into. What do you think?


Reply to this email directly or view it on GitHub:
https://github.com/angular-ui/router/issues/1#issuecomment-13293257

@timkindberg
Copy link
Contributor

I'd like to close this issue to urge people to start logging new issues (one issue per singular topic/bug/idea)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests