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

feat(router): provide RouteConfig object for AuxRoute #4319

Closed
btford opened this issue Sep 22, 2015 · 15 comments
Closed

feat(router): provide RouteConfig object for AuxRoute #4319

btford opened this issue Sep 22, 2015 · 15 comments
Labels
effort1: hours feature Issue that requests a new feature

Comments

@btford
Copy link
Contributor

btford commented Sep 22, 2015

Right now, you must do:

@RouteConfig([
  new AuxRoute({ path: '/foo', component: FooCmp })
])

To use auxiliary routes. We should provide a more ES5-friendly version like:

@RouteConfig([
  { auxPath: '/foo', component: FooCmp }
])

Or:

@RouteConfig([
  { path: '/foo', auxComponent: FooCmp }
])

I'm open to suggestions.

@0x-r4bbit
Copy link
Contributor

Without thinking about it too much, I'd vote for something like:

@RouteConfig([
  { path: '/foo', component: FooCmp, aux: true }
])

@btford
Copy link
Contributor Author

btford commented Sep 22, 2015

I think that's a bad option. Consider scanning down a list of routes:

@RouteConfig([
  { path: '/foo', component: FooCmp, data: { ... } },
  { path: '/bar', component: FooCmp },
  { path: '/bar/:id', component: FooCmp, as: 'FooCmp' },
  { path: '/bar', component: FooCmp, aux: true },
  { path: '/baz/:id', redirectTo: '/' }
])

You can't easily tell when scanning this list which ones are auxiliary. For instance, it looks like a mistake that you have two routes with path /bar, when it's in fact a valid config because one is auxiliary. You won't notice this until you scan all the way right through the different options.

@0x-r4bbit
Copy link
Contributor

Hm.. I understand. But it feels inconsistent otherwise.. Thing is, that other route definitions like Redirect for example, also just introduce another property which is (prolly) used to determine the route definition type. It just made sense to me to introduce such a property for aux route too, while keeping the rest consistent..

@PatrickJS
Copy link
Member

what do you think of a type property?
{type: 'aux'}|{type: 'redirect'}|{type: 'async'}

@btford
Copy link
Contributor Author

btford commented Sep 23, 2015

@gdi2290 I think that suffers the same problem I mentioned in #4319 (comment)

Each other type introduces a new "target" (redirectTo, component, etc.) while keeping "path". Aux routes are different because the meaning of their "path" is different. For this reason, I think something about path (or whatever property comes first) needs to be different.

Things that behave similarly should look similar. Things that do not behave similarly should not look similar. This case is the latter.

@PatrickJS
Copy link
Member

@btford ya that's a good point. I also agree @PascalPrecht that it is inconsistent otherwise. The benefit of my proposal is that it's following a javascript convention for the most part. For example, we can look at the GeoJson spec

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [125.6, 10.1]
  },
  "properties": {
    "name": "Dinagat Islands"
  }
}

notice how there are two types here as it's one way to serialize types. We soon realized that this convention may result in maintainability problems when communicating between two typed languages, other than javascript, which lead to the creation of transit spec https://github.com/cognitect/transit-format

these routes presumably will remain small (larger apps would end up using types e.g. new AuxRoute({})) so there won't be any maintainability problems. The type prop also allows for future proof since the value is dynamic and new types (if any) could be added.

@0x-r4bbit
Copy link
Contributor

I think a more abstracted way is better here. I'm not sure if that's possible, but what if there's a new route definition type that comes up in the future? Do we need to introduce a new property then? This sure depends on the semantics of that particular definition type then, but if we had decided for something like a simple flag, or a type property (which I even like more), we don't have to introduce any new APIs.

I think from a consumer point of view, who doesn't have such a deep understanding of the implementation and semantics itself, will be confused why there's such a special property for the thing called aux route.

@Berkana
Copy link

Berkana commented Sep 25, 2015

@btford the fix is to teach good conventions, and to have people put the aux: true property first. Then it is more visually apparent at a glance, and still more succinct.

The tension between typing a whole lot more to remove potential confusion and typing less for the folks who use a feature a lot is resolved by this convention. If you have to remember the convention of including aux: true first, it's like having to remember to do new AuxRoute(...). But the convention entails a lot less typing.

I like the idea of having good conventions add clarity. I miss having the ability to use ! and ? in variable names, from Scheme. In Scheme, all predicates had names ending in ? by convention, and all functions that mutate a value had names ending !. There were a lot of smart little conventions that helped make the remembering of crucial details easier. These should be embraced; maybe even a Sublime plug-in could be written to flag the failure to adhere to these conventions.

The nice thing about this convention is that the code will work even if aux: true is not put first. It is not brittle.

@Berkana
Copy link

Berkana commented Sep 25, 2015

Actually, let me amend my recommendation. Adding a type property would be better than aux: true. But I would also add that it should be a convention that the type is entered first, as a best practice.

@gkalpak
Copy link
Member

gkalpak commented Sep 25, 2015

I haven't played with the new router at all (is it still "new" ? :P), but would it make sense to overload the @RouteConfig decorator like this:

@RouteConfig({
  routes: [
    {.../* route 1 */...},
    {.../* route 2 */...},
    ...
  ],
  auxRoutes: [
    {.../* aux route 1 */...},
    {.../* aux route 2 */...},
    ...
  ],
  whateverTypeOfRoutes: [
    {.../* whatever type of route 1 */...},
    {.../* whatever type of route 2 */...},
    ...
  ]
})

And have the current (most common ?) form be a shortcut. E.g.:

@RouteConfig([.../* routes */...])

// is equivalent to (and a shortcut for):

@RouteConfig({
  routes: [.../* routes */...]
})

This proposal might not make sense at all, but hey 😐

@Berkana
Copy link

Berkana commented Sep 25, 2015

@gkalpak I like that one you suggested. It makes the types explicit, but doesn't require instantiation to be done by the user at the time something is passed into RouteConfig.

@btford
Copy link
Contributor Author

btford commented Oct 16, 2015

I'm not opposed to @gkalpak's suggestion, but it seems quite verbose.

For now, I'm going to go with:

@RouteConfig([
  { aux: '/foo', component: FooCmp }
])

We can always revisit the name later if it proves too confusing.

@thelgevold
Copy link
Contributor

Are aux routes akin to named sub views in UI-router?

With the proposed solution, the confusing part for me is that it's not immediately clear how the aux routes relate to the primary route from the router definition alone.

I was thinking about the following as an alternative syntax:

//Router def
@RouteConfig([
  { path: '/foo', component:mainComponent ,outlets:['outlet1','outlet2']},
  { path:'/view1, component:Cmp1', as:'View1'},
  { path:'/view2, component:Cmp2', as:View2},
])

//Inside mainComponent
<div id='mainViewContainer'>
  //Cmp1 renders here
  <router-outlet view='outlet1'></router-outlet>

  //Cmp2 renders here
  <router-outlet view='outlet2'></router-outlet>
</div>

I guess my point is that I am not seeing the need to distinguish between aux vs "regular" routes since any route could potentially serve both purposes. Basically any route can be configured to be a standalone route or a "named view" route if it's added to the outlets array.

Assuming the urls would still be the same format as in the Angular connect presentation where the sub views are () parameters to the primary: /primary/(named-view-1)...(named-view-n)/

@btford
Copy link
Contributor Author

btford commented Oct 30, 2015

Are aux routes akin to named sub views in UI-router?

No. More docs forthcoming.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort1: hours feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

7 participants
@thelgevold @0x-r4bbit @btford @PatrickJS @Berkana @gkalpak and others