aurelia / aurelia

Aurelia 2, a standards-based, front-end framework designed for high-performing, ambitious applications.
MIT License
1.39k stars 150 forks source link

[RFC] [WIP] Dynamic routing #307

Closed jwx closed 8 months ago

jwx commented 5 years ago

💬 RFC

A proposal for a feature set where the router doesn't use traditional url/path routes.

🔦 Context

A lot of the trickier part of routing -- route recognizing, child routing, defining navigation statically -- is related to routes. By removing the routes, we can get rid of some of the issues as well.

💻 Examples

A fully functional, routeless router would have the following url

/viewport:component/=parameters (not sure about the parameters part yet)

Typical examples would be:

/default:start-page  -- just a simple start page in the default viewport
/master:users/detail:user/=id=123 -- a master-detail in two viewports

Now this can be simplified:

Same examples would then be:

/start-page  -- just a simple start page in the default viewport
/users/user/=123 -- a master-detail in two viewports

The key would be to get to decisions made by convention and as little configuration as possible. Whenever there's only one option left, the router will be able to make the decision. So, more ways to eliminate viewport options are

davismj commented 5 years ago

Seems like this would shut the door on using the same component in two separate viewports.

fkleuver commented 5 years ago

Could the viewport property be of type string | string[]?

EisenbergEffect commented 5 years ago

I confess to be having a hard time understanding why we would invent a new string-based syntax for routes. For example, if we want a developer to be able to tell the router how to adhoc navigate to a specific component and viewport config, why not create a router API directly for that? If they need to generate links for that, why not just have them use a custom attribute to configure the params for the same underlying router API?

In other words, what is the real end goal we're trying to get here and is a custom string format the best way to do that?

fkleuver commented 5 years ago

The way I see it (@jwx correct me if I'm wrong): Say you've got a normal Aurelia app. No routing configured whatsoever. Just components. With this url format, you can tell the router to load a particular component in a particular viewport, with/without parameters. with an unconfigured router So regardless of the alternatives, those alternatives have one thing in common: something needs to be declared somewhere in the code base, which is not needed with this format.

In that sense it's the most dynamic kind of routing you can have. And I see some benefits there:

EisenbergEffect commented 5 years ago

So, it's the syntax that I'm getting hung up on a bit I guess. I'm more interested in the underlying API for that right now and how that would work.

jwx commented 5 years ago

Seems like this would shut the door on using the same component in two separate viewports.

It doesn't. In fact, my first test with this in the test application placed the same component in two separate viewports.

jwx commented 5 years ago

Could the viewport property be of type string | string[]?

Yes, definitely. Not a problem at all.

jwx commented 5 years ago

@EisenbergEffect The underlying api is the foundation. The string syntax is just a way to be able to use browser <a> links and have deep links into the application via the url. In an app without those browser characteristics, strings might not be used at all.

The main part of the (wip) api is:

goto(views: string | string[] | Object, parameters?: Object, title?: string)

Only information that the Router can't figure out through router, viewport and component configuration and conventions need to be provided. Here are some examples:

this.router.goto('welcome') // Only one single available viewport (possibly through configuration/convention)

this.router.goto(Welcome) // Same as previous, but with component

this.router.goto([ 'inbox', PreviewEmail ], { id: 123 }) // Two single available viewports, passing parameter to both components 

this.router.goto({ master: 'inbox', detail: PreviewEmail }, { id: 123 }) // Same as above, but not single available viewports so specification needed

this.router.goto({ 
    master: { component: 'inbox', parameters: { filter: 'aurelia' } }, 
    detail: { component: PreviewEmail, parameters: { id: 123 } }
}, { language: 'sv' })
// Individual parameters sent to each component unioned with shared parameters

And of course

this.router.goto('/inbox/preview-email') // Using the string syntax for everything

Names, interfaces, parameter orders and similar is far from set.

To be continued...

jwx commented 5 years ago

I'll provide an actual example. If we've got two components, Abc and Def, that we want to show in a viewport when a link and a button are clicked respectively, it could look like this:

First the Abc component (and Def is identical)

<!-- abc.html -->
<template>
  <div>Hello! I'm Abc.</div>
</template>
// abc.ts
import { customElement } from '@aurelia/runtime';
import * as template from './abc-component.html';

@customElement({ name: 'abc', template })
export class Abc { }

The components are registered with DI (a vNext requirement according to @fkleuver)

// startup.ts or main.ts or plugin configure or similar
container.register(
  <any>Abc,
  <any>Def,
);

After this, only App remains

<!-- app.html -->
<template>
    <a href="#/abc">Abc</a>
    <button click.trigger="clickDef()">Def</button>

    <viewport></viewport>
</template>
// app.ts
import { inject } from '@aurelia/kernel';
import { customElement } from '@aurelia/runtime';
import * as template from './app.html';
import { Router } from '@aurelia/router';
import { Def } from './def';

@inject(Router)
@customElement({ name: 'app', template })
export class App {
  constructor(private router: Router) {
    this.router.activate();
  }

  clickDef() {
    this.router.goto(Def);
  }
}

And that's it. No additional configuration or setup needed. The link and the button now provide full routing functionality, including browser buttons.

davismj commented 5 years ago

What happens to the browser url when you go to DEF?

jwx commented 5 years ago

What happens to the browser url when you go to DEF?

It's set to #/def.

jwx commented 5 years ago

Another example: If you've got an application with two features, email and calendar, who's got their own links to components, it could look like this:

<!-- app.html -->
<template>
  <a href="#/email">Email</a>
  <a href="#/calender">Calendar</a>

  <viewport name="application" used-by="email,calendar"></viewport>
</template>
<!-- email.html -->
<template>
  <a href="#/inbox">Inbox</a>
  <a href="#/contacts">Contacts</a>
  <a href="#/about-application">About application</a>

  <viewport name="email"></viewport>
</template>
<!-- calendar.html -->
<template>
  <a href="#/dates">Dates</a>
  <a href="#/schedule">Schedule</a>

  <viewport name="calendar"></viewport>
</template>

And again, no other setup or configuration required. It'd just work.

And if calendar wanted to show the component about-application from the email feature, calendar would only need to add

  <a href="#/about-application">About application</a>

for it to work.

bigopon commented 5 years ago

The above example has slot like behavior, quite different and very nice!

EisenbergEffect commented 5 years ago

This is helping to clarify things a bit more. @bigopon brings up an interesting point. It makes me wonder, could this all actually be done with slots?

<template>
  <a href="#/email">Email</a>
  <a href="#/calender">Calender</a>

  <slot name="application" used-by="email,calender"></slot>
</template>

You could then just instantiate the element hierarchy and let it render. Not sure if it would really work, but it's so similar.

jwx commented 5 years ago

@EisenbergEffect I'm guessing the viewports are similar to slots, replaceables and composes. It's not surprising, since the viewports are also markers for inserted content. But as far as I know, there are some differences, for example the life cycle and being a target before being defined. I'm thinking that if we find this is worth pursuing I'll initially implement it with the viewport. Once we know exactly what we need from it, we can then see if there's an already existing good fit.

EisenbergEffect commented 5 years ago

That sounds good to me :)

EisenbergEffect commented 5 years ago

I think you should continue to explore this avenue.

jwx commented 5 years ago

A few words on

Viewport scope

Viewports for most applications will live within a single viewport scope (tied to the Aurelia root). Regardless of sibling and parent-child relationships, they will, from the router's perspective, be equal, using the name as their identifier.

There might however be situations where this will not work, recursiveness and name collisions (possibly due to features/plugins) comes to mind. The solution to this is creating a new viewport scope by adding a scope attribute to the viewport custom element.

<viewport name="sub-application" scope></viewport>

This will have the result that the above viewport is a part of the parent scope, but all viewports inside it will be a part of a new scope. In this new scope, all viewports' names are automatically prefixed with the scope prefix (basically the "viewport path" to the sub-application viewport) giving them unique identifiers and allowing navigation within and between viewport scopes.

@fkleuver @davismj @EisenbergEffect @bigopon

EisenbergEffect commented 5 years ago

I fear that this design won't work for large, modular apps and teams as they will need to know about the details of other module internals. Shouldn't all viewports be scoped?

jwx commented 5 years ago

I fear that this design won't work for large, modular apps and teams as they will need to know about the details of other module internals. Shouldn't all viewports be scoped?

Well, they will all be scoped since there's an app scope.

I'm not sure I understand how this would change the level of knowledge you need to have into modules. If you want to target a viewport within a module, you'll need to know its name. That's no different than the route-view element in vCurrent. @EisenbergEffect

davismj commented 5 years ago

My first thought is I don't think it's necessary. All routes are already scoped by their parent property, in the recursive case. So a route with parent Foo can only be loaded into a viewport in the Foo view.

In the collision case, I think we can defer to developers the same way we do for custom elements. It's up to you to be clever in naming components you expect to be used by others. Same for viewports.

On Mon, Nov 26, 2018, 09:21 Jürgen Wenzel <notifications@github.com wrote:

I fear that this design won't work for large, modular apps and teams as they will need to know about the details of other module internals. Shouldn't all viewports be scoped?

Well, they will all be scoped since there's an app scope.

I'm not sure I understand how this would change the level of knowledge you need to have into modules. If you want to target a viewport within a module, you'll need to know its name. That's no different than the route-view element in vCurrent.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aurelia/aurelia/issues/307#issuecomment-441487522, or mute the thread https://github.com/notifications/unsubscribe-auth/ADquv0axpa1iExBblFKx8hyP73RH3pA5ks5uyzQXgaJpZM4YsQNY .

jwx commented 5 years ago

@davismj I agree that it's primarily up to developers to sort out their naming. Also, while viewport scope might provide some help with this, it's still totally possible to mess everything up so some thought and planning will still be required.

As for the the recursive case, that limitation isn't true for my routeless router. It's not hard to let any component be loaded into any viewport. Viewport scopes, or something similar, are however necessary for recursiveness to work.

And as an added bonus, viewport scopes can be used to resolve name conflicts between modules (even though it shouldn't be necessary).

davismj commented 5 years ago

On Mon, Nov 26, 2018, 10:13 Jürgen Wenzel <notifications@github.com wrote:

@davismj https://github.com/davismj I agree that it's primarily up to developers to sort out their naming. Also, while viewport scope might provide some help with this, it's still totally possible to mess everything up so some thought and planning will still be required.

As for the the recursive case, that limitation isn't true for my routeless router. It's not hard to let any component be loaded into any viewport. Viewport scopes, or something similar, are however necessary for recursiveness to work.

Thats a good sign to me that, while cool, a routeless router may not be well defined and practical for actual applications.

And as an added bonus, viewport scopes can be used to resolve name conflicts between modules (even though it shouldn't be necessary).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aurelia/aurelia/issues/307#issuecomment-441491854, or mute the thread https://github.com/notifications/unsubscribe-auth/ADquvyXlGyXVCsRK8Nr8eEbpQtzAty-qks5uy0AxgaJpZM4YsQNY .

Vheissu commented 5 years ago

At first, the concept of a routerless router was confusing to me, but having read through all of the above and the examples/code itself, I like where this is going. It's different, but I would really love to see this explored further.

The vCurrent router feels out of place in terms of Aurelia's mantra of, "convention over configuration" every aspect of Aurelia adheres to this mantra, ironically except the router which requires a lot of configuration to get running in your applications.

The approach @jwx is going down results in a lot less code and appears to be more flexible to me in what you can do with it. Case-in-point, to define a router pipeline step at the moment it feels like a lot of code when really all you need is a function which decides if the route can proceed, cancel or redirect elsewhere.

@EisenbergEffect Curious what your thoughts are on this? I've been working with routes a lot lately and I can see this making the experience of working with routes so much nicer and simpler.

@davismj Curious what you mean by not well defined and practical? Is it because this approach is less verbose and explicit compared to the vCurrent router? If so, this is one of my gripes with the vCurrent router, it feels like a lot of unnecessary typing to define routes and add in pipelines.

davismj commented 5 years ago

@Vheissu No, I don't care for vCurrent. I agree with your observations as well. It's very much out of line with the Aurelia vision.

fkleuver commented 5 years ago

Thats a good sign to me that, while cool, a routeless router may not be well defined and practical for actual applications.

@davismj Can you clarify a little more precisely what you're referring to with the word "Thats" in this sentence? What specifically is a good sign that it may not be practical?

jwx commented 5 years ago

Some words on

Navigation flow

There's a fair amount of steps that happen when navigation is initiated. The general principle of the flow is this:

1a. User clicks an anchor link, or 1b. Navigation is initiated from code with goto 2a. The link handler (for now) adds relevant scope to the anchor's href string and notifies the history browser 2b. The router translates the goto parameters to the anchor equivalent and notifies the history browser

  1. The history browser updates the location, adding to the navigation history
  2. The location is changed, either by a previous step or by browser buttons or manual typing
  3. The history browser reacts to the location change and passes it, together with navigational flags, to the router
  4. The router resolves the location string into viewport states and acts upon them by either instructing history browser to cancel the navigation (at which point the navigation history is restored), redirecting it (by instructing history browser to replace the location, triggering 4) or continue by setting content in the viewports
  5. Viewports load their new contents
  6. The router gathers the total viewport state and instructs history browser to set the location without triggering a navigation on the change

This flow is the same regardless of triggering action, the only thing that differs is the entry point into the flow. Navigation guards and lifecycle events are interspersed throughout the flow.

@fkleuver @davismj @EisenbergEffect @bigopon

davismj commented 5 years ago

@fkleuver the need for scoping viewports. It doesn't track an actual concept you would use when thinking about how you'd want to build your application. It's a necessity added to address a technical limitation. It adds a lot of unnecessary complexity.

@jwx nice outline. I'm not sure if all those steps will work exactly the way you describe, particularly (3) and (4), but I'll leave you to figure it out.

jwx commented 5 years ago

@davismj Oh, the flow I described is the way it behaves right now (in the PR). While I've left out technical details, it does indeed work. (Yes, @fkleuver, I will write the tests that prove it.) Sorry for being unclear about that.

The need for some kind of scoping arises when you want recursive viewports, that is viewports that contain versions of themselves (like in the navigation skeleton), if you want to have the ability to reach into different levels of the hierarchy and still keep the same principles for identifying viewports regardless of where in the hierarchy they are. Exactly that same need is present in vCurrent. In vCurrent it's solved by adding and configuring a child router. In this router it's solved by adding an empty scope attribute on the viewport that can contain recursive versions of itself. I personally find "configure a child router for the view model that owns the router-view where you want the recursive content" to be more complex than "add an empty scope attribute on the viewport where you want to be able to have recursive content".

I'd like to point out that nothing extra needs to be done, not even adding a scope attribute, to handle non-recursive viewports within viewports. There's no need at all for child routers in this router; viewports within viewports just work out of the box.

davismj commented 5 years ago

I will write the tests that prove it.

Very much looking forward to this.

I personally find "configure a child router for the view model that owns the router-view where you want the recursive content" to be more complex than "add an empty scope attribute on the viewport where you want to be able to have recursive content".

Yes, it's better than vCurrent. That's a low bar to set.

jwx commented 5 years ago

Yes, it's better than vCurrent. That's a low bar to set.

Well, it was the only bar regarding recursive content I could find when looking at vCurrent and the vNext specs.

jwx commented 5 years ago

Something short regarding

Hooks

There are five levels where we might want to apply hooks. Some of these are less probable than others, but let's at least list them. 1) On the router Hooks added to the router are applied to every navigation. These correspond to the current pipeline steps. 2) On the viewport Hooks added to a viewport are applied to every navigation within that viewport. 3) On the component Hooks added to a component are applied to every navigation that involves the component. 4) On a combination of components Hooks added to a combination of components are applied to every navigation that involves all the components in the combination. Not sure this will really be necessary. 5) On a specific navigation Hooks added to a specific navigation are only applied on that specific navigation. Not sure there are any real use cases for this that aren't covered by the previous ones.

Hooks can be added with router api (all of the above), with html attributes (primarily viewport), decorators/static variables (primarily components) or in navigation code (primarily specific navigation).

The router api will, among other things, allow specifying components as DI objects and by names (which include wildcards) and possible exclusions. It could look something like this

router.addHook({ components: '*', exclude: [ 'About', Login ] }, authHook);

All hooks should be chainable where execution order is relevant. Hooks can return either a new navigation or the next hook in the hook chain.

Thoughts and opinions? @davismj @EisenbergEffect @fkleuver @bigopon @BBosman @Vheissu

EisenbergEffect commented 5 years ago

Let's try to avoid using the term "module" as vNext doesn't do module loading or really know anything about modules. Instead, we should use component terminology.

jwx commented 5 years ago

Let's try to avoid using the term "module" as vNext doesn't do module loading or really know anything about modules. Instead, we should use component terminology.

Yes, of course. Don't know where that came from (it's not there in anything else).

davismj commented 5 years ago

On the component

This is activation. I don't think we want to change activation.

On a combination of components

That's a route. Let's call it a route so we're all on the same page.

On a specific navigation

I don't know what this means, what it looks like, or how you would configure it.

router.addHook({ components: '*', exclude: [ 'About', Login ] }, authHook);

Here's what I had specced out.

const authCheck = (to, from, next): RouteGuard => {
  const isAdmin = false;
  if (!isAdmin) {
    return next({ redirect: 'home' });
  }
  return next();
};
const timeoutCheck = (to, from, next) => {
  const timedOut = true;
  if (timedOut) {
    return next({ name: 'login' });
  }
  return next();
};

router.before = [
  timeoutCheck,
  (to, from, next) => {
    const index = Math.random() * 100 % router.navigation.length;
    const route: IRouteConfig = router.navigation[index].config;
    return next(route);
  }
];
router.mapRoutes([
  {
    path: 'foo',
    component: Foo,
    before: [
      authCheck,
      (to, from, next) => next({ redirect: 'home' })
    ]
  },{
    path: 'bar',
    component: Bar,
    before: [authCheck]
  },{
    path: 'welcome',
    component: Welcome
  }
]);

The question is how do you handle (1) router.before and (2) route.before in an unconfigured router. Note the difference between (2) and activation is that it applies to many but not all routes, which is a common use case for authentication. Your router.addHook approach above uses configuration, at which point it probably should a lot more like the spec.

jwx commented 5 years ago

This is activation. I don't think we want to change activation.

I was thinking about an addition to activation. I agree we don't want to change activation.

That's a route. Let's call it a route so we're all on the same page.

Sure, it covers the current route functionality, but it's more than that. It's any navigation that contains or results in a specific combination of components.

I don't know what this means, what it looks like, or how you would configure it.

You wouldn't configure it, it'd be added in the navigation code. Something like this (NOTE: not a finished spec);

router.goto(components, parameters, { before: specialCaseFunction });
  • I believe we should avoid magic strings for component names.

I think it's worth the price.

  • I feel that the name addHook is a bit unclear, specifically concerning order and scope.

I agree that it needs to be more specific. It was just an example to try and illustrate a principle.

  • I feel that this approach of selecting and excluding components is trying to do too much and will cause problems specifically for application maintainers as development teams add more components.

I think it's quite the opposite: an easy and maintainable way to include and exclude components in guard behaviors.

Here's what I had specced out.

I'm familiar with the spec you've made, but intentionally stayed away from referring it since I wanted this to be regarding principles and needs, rather than implementation details.

The question is how do you handle (1) router.before and (2) route.before in an unconfigured router.

I think there might be a misunderstanding here. I've never said it shouldn't be possible to configure the router and it's never been my intention to not support router configuration. What I have said is that it shouldn't be necessary to configure the "traditional path routes"

{ name: 'internalName', route: 'path/indicating/functionality/', viewports: { 'main':  Component } }

in order to simply navigate to Component in main.

Note the difference between (2) and activation is that it applies to many but not all routes, which is a common use case for authentication. Your router.addHook approach above uses configuration, at which point it probably should a lot more like the spec.

Yes, the configuration parts should probably take as much advantage as possible of the pre-existing work. But I prefer to start with principles and once they are "set" apply the relevant parts of the spec. I think it's quite valuable to be able to say "apply authentication checks on everything except these three components" rather than having to handle that by specifying all components it should be applied to or by having the authentication code exclude the three components. I think it's a principle that's worth discussing and I think it's easier to come to conclusions from that point of view rather than based on a spec that doesn't have it.

davismj commented 5 years ago

I was thinking about an addition to activation. I agree we don't want to change activation.

What does this solve that activation doesn't?

Sure, it covers the current route functionality, but it's more than that. It's any navigation that contains or results in a specific combination of components.

Ok, I see what you're getting at. I think once you wrote out an example of how to do this, though, you might find it would become too complicated for common use cases.

I could imagine use cases where you have a general purpose component that you wanted to use for various kinds of data. Maybe when you had User and Configuration together, auth is required, but Example and Configuration together it isn't.

You might be making a case for a powerful low level api but without examples it's hard to say for sure.

You wouldn't configure it, it'd be added in the navigation code. Something like this (NOTE: not a finished spec);

router.goto(components, parameters, { before: specialCaseFunction });

Why wouldn't you use

specialCaseFunction();
router.goto(components, parameters);

Can you think of a valid use case for this?

I think it's worth the price.

Why?

I think it's quite the opposite: an easy and maintainable way to include and exclude components in guard behaviors.

router.addHook({ components: '*', exclude: [ 'About', Login ] }, authHook);

I have two concerns. Let's say you have 100 components total and 10 of them are not authed. (1) Since the configuration is imperative, not declarative, it will be a maintenance headache for changing class names. (2) A typical application might have three of these hooks serving different scopes, meaning that these lists could become pretty tedious to maintain.

router.addHook({ components: '*', exclude: [ HomeComponent, LoginComponent, RegisterComponent, ConfirmComponent, ForgotPasswordComponent, ResetPasswordComponent, PricingComponent, SubscribeComponent, AboutComponent, HelpComponent ] }, authHook);

Rather than adding this functionality, which people will use and abuse, it might be better to offset the responsibility of identifying hooked components to the hook itself.

const authCheck = (to, from, next): RouteGuard => {
  const requiresAuth = to.components.some((component) => component.meta.requiresAuth);
  const isAdmin = false;
  if (!isAdmin) {
    return next({ redirect: 'home' });
  }
  return next();
};

I'm familiar with the spec you've made, but intentionally stayed away from referring it since I wanted this to be regarding principles and needs, rather than implementation details.

My specs are full of actual use cases. Your RFC here has no use cases. The least you can do is copy and paste some use cases in to give your ideas some context.

The question is how do you handle (1) router.before and (2) route.before in an unconfigured router. I think there might be a misunderstanding here. I've never said it shouldn't be possible to configure the router and it's never been my intention to not support router configuration. What I have said is that it shouldn't be necessary to configure the "traditional path routes"

{ name: 'internalName', route: 'path/indicating/functionality/', viewports: { 'main': Component } }

in order to simply navigate to Component in main.

Ok. So, if you navigate to main without configuring a route, the question is still how do you handle (1) router.before and (2) route.before in an unconfigured router.

I think it's quite valuable to be able to say "apply authentication checks on everything except these three components" rather than having to handle that by specifying all components it should be applied to or by having the authentication code exclude the three components. I think it's a principle that's worth discussing and I think it's easier to come to conclusions from that point of view rather than based on a spec that doesn't have it.

Ok. Why?

jwx commented 5 years ago

Let me start with something that should probably have been the start of the original post on hooks: this is/was intended as a list of points/levels/objects/whatever where I thought hooks could be relevant, starting from the widest, router, to the most narrow, a specific navigation. Furthermore, I like consistency and I like offering more than one way to do things (I know that combination might seem a bit strange), so I envisioned that while hooks would behave the same, you'd be able to get the same result by adding hooks on different levels (an auth hook on the router, the viewport or the component would all result in an auth check before a component is loaded in a viewport). One of the things I like with Aurelia is that it gives me a lot of rope and then the rest is up to me, so for me it feels natural to continue handing out rope.

I'd also like to point out that I don't think there isn't a single, objective truth when it comes to how developers should structure their code/components and what should be placed where. Some will prefer to have auth configuration on/in the actual component (like a decorator or static variable) and some will want to delegate that to something with a higher vantage point in the system (like a router auth configuration for all components that can be navigated to). If it works for them and those who will be doing maintenance, then fine, go ahead. (Yes, this also extends to using webpack and React and similar... ;) )

I was thinking about an addition to activation. I agree we don't want to change activation.

What does this solve that activation doesn't?

It gives the ability to add hooks (that are chainable and so on) to a component without the component actually having to know about it or have code for it. I think this is a common desire, for example when it comes to auth.

Sure, it covers the current route functionality, but it's more than that. It's any navigation that contains or results in a specific combination of components.

Ok, I see what you're getting at. I think once you wrote out an example of how to do this, though, you might find it would become too complicated for common use cases.

I could imagine use cases where you have a general purpose component that you wanted to use for various kinds of data. Maybe when you had User and Configuration together, auth is required, but Example and Configuration together it isn't.

You might be making a case for a powerful low level api but without examples it's hard to say for sure.

Yes, this is a good example of what it could be used for but I'm not sure it'd really be common enough to warrant a possible hook on this level. Probably not in the alpha release, at least.

You wouldn't configure it, it'd be added in the navigation code. Something like this (NOTE: not a finished spec); router.goto(components, parameters, { before: specialCaseFunction });

Why wouldn't you use

specialCaseFunction();
router.goto(components, parameters);

Can you think of a valid use case for this?

It'd have the same cancellation/redirect opportunities as a hook on other levels, but I think you're right in that it'd just be achieved by making the goto conditional to the outcome of specialCaseFunction. I haven't been able to think of a use case, but included it anyway for completeness.

I think it's worth the price.

Why?

Because being able to match a set of components will keep the lists smaller and add the "all these" and "all but these" options.

I think it's quite the opposite: an easy and maintainable way to include and exclude components in guard behaviors.

router.addHook({ components: '*', exclude: [ 'About', Login ] }, authHook);

I have two concerns. Let's say you have 100 components total and 10 of them are not authed. (1) Since the configuration is imperative, not declarative, it will be a maintenance headache for changing class names. (2) A typical application might have three of these hooks serving different scopes, meaning that these lists could become pretty tedious to maintain.

I must be missing something, because I can't find any maintenance headache here. Sure, if the 10 classes are changed, you'll have to let the configuration follow and there could be several lists, yes. But most of the time for me when I change class names, it's not troublesome enough to be a maintenance headache.

router.addHook({ components: '*', exclude: [ HomeComponent, LoginComponent, RegisterComponent, ConfirmComponent, ForgotPasswordComponent, ResetPasswordComponent, PricingComponent, SubscribeComponent, AboutComponent, HelpComponent ] }, authHook);

Rather than adding this functionality, which people will use and abuse, it might be better to offset the responsibility of identifying hooked components to the hook itself.

I don't have any problems at all with your solution below. As I said before I like to give people a lot of (different kinds of) rope. The fact that some people might hang themselves with it doesn't mean I want to take rope away from those who won't. In other words, I think both should be fine and easily achieved.

This is however a question that's more general: To what degree do we enforce our beliefs of what is good and right on the developers via the framework? Where do we draw the line to protect them from themselves? And, eventually, on which side of the line would the above functionality fall?

My specs are full of actual use cases. Your RFC here has no use cases. The least you can do is copy and paste some use cases in to give your ideas some context.

Fair point. Although for some ideas/thoughts I don't even have use cases. I should probably be able to provide at least more context.

Ok. So, if you navigate to main without configuring a route, the question is still how do you handle (1) router.before and (2) route.before in an unconfigured router.

I can't without some routeR configuration. Examples (to illustrate, not spec)

router.addComponentHook(Router.Hooks.before, Main, authHookFunction);

authHookFunction is always called before Main is navigated to, regardless of viewport and anything else.

router.addViewportHook(Router.Hooks.before, 'content', authHookFunction);

always called before anything is navigated to in the content viewport.

router.addHook(Router.Hooks.before, { exclude: [ Login, 'welcome' ] }, authHookFunction);

always called before anything except components Login and 'welcome' is navigated to, regardless of viewport and so on.

In addition to that, @decorators and static variables on the component should lead to the same result (again, just an illustration, not a finished spec):

@hooksBefore([ authHookFunction ])
export class Main { ... }

I think it's quite valuable to be able to say "apply authentication checks on everything except these three components" rather than having to handle that by specifying all components it should be applied to or by having the authentication code exclude the three components. I think it's a principle that's worth discussing and I think it's easier to come to conclusions from that point of view rather than based on a spec that doesn't have it.

Ok. Why?

Not sure I understand the question, but I'm going to give the "rope answer" another shot.

davismj commented 5 years ago

I don't have any problems at all with your solution below. As I said before I like to give people a lot of (different kinds of) rope. The fact that some people might hang themselves with it doesn't mean I want to take rope away from those who won't. In other words, I think both should be fine and easily achieved.

If you're talking about power, such as the ability to do x, y, and z, I agree. But when it comes to writing an API, you want the minimal and simplest set of APIs such that x, y, and z can be accomplished. Otherwise, first you end up with (a) a maintenance headache and regression nightmare, as each new API you introduce will need to be documented, maintained, and marginally consistent with other APIs. Second, you introduce (b) confusion and maintenance issues for developers and particularly development teams, as teams will not be sure the difference between various tools, will try to learn the difference between them, and may even end up using different tools on the same application independently, which is confusing.

A good example of (a) is layouts. They do nothing that compose and setRoot doesn't do, and they've added considerable work for me to support. (You might argue that layouts do setRoot better than setRoot. Then keep layouts and get rid of setRoot. The argument stands).

A good example of (b) is child routers. Nine times out of ten child routers can and should be avoided, but the name "child routes" best fits the mental image of what developers are trying to accomplish. Yet they introduce a great deal of complexity as well as limitation on what the developer can accomplish, e.g. navigating from a child of one parent to a child of another parent using route-href. The majority of the questions asked on discord or stack overflow are issues with child routes.

By avoiding (a), we are able to focus more time and attention on developing new features or fixing critical bugs. By avoiding (b), we increase developer happiness and strengthen Aurelia's reputation as the developer favorite front-end framework. I hope this is understandable and that you share the goal of providing the min-max of optimal features with simplest API.

This is however a question that's more general: To what degree do we enforce our beliefs of what is good and right on the developers via the framework? Where do we draw the line to protect them from themselves? And, eventually, on which side of the line would the above functionality fall?

To answer your question: By providing the simplest and least restrictive API possible, we enforce fewer of our beliefs on developers. That's why I'm pushing for it.

This particular discussion is not about enforcing something on the developers. It is about creating a simple, clean, understandable, readable, extensible API. That is our job, and that is what we're talking about here. Not limiting a developer's ability to do something.

As Aurelia engineers, it is our job to design the extensiblity points to expose to developers, and developers are forced to hook in them. This is why we want to min-max the maximum features with the simplest API. Aurelia is as popular as it is today not just because it is powerful but because its API--for example the binding API--is simple, clean, understandable, readable, and extensible.

In perspective, you are or should be saying the following code is the simplest way to achieve the largest feature set:

router.addHook({ components: '*', exclude: [ 'About', Login ] }, authHook);

I'm challenging that notion by saying when component counts are large, this will not be simple. I gave an example.

// adding or removing a route requires adding a new item to this list every time
router.addHook({ components: '*', exclude: [ HomeComponent, LoginComponent, RegisterComponent, ConfirmComponent, ForgotPasswordComponent, ResetPasswordComponent, PricingComponent, SubscribeComponent, AboutComponent, HelpComponent ] }, authHook);

Compare with:

const authCheck = (to, from, next): RouteGuard => {
  // adding or removing a route requires declaratively opting a component into auth
  const requiresAuth = to.components.some((component) => component.meta.requiresAuth);
  const isAdmin = false;
  if (!isAdmin) {
    return next({ redirect: 'home' });
  }
  return next();
};
EisenbergEffect commented 5 years ago

On this particular issue of hooks, I think what @davismj is proposing is the best way to go. It has the greatest possible flexibility (you can literally do anything and have any custom logic) and it's also familiar to both front-end and back-end developers as the API is nearly identical to most middleware APIs in other routing solutions. Not only is it familiar, but it's a proven pattern that we know works at any scale.

The declarative approach proposed by @jwx could be built on top of the API that @davismj is proposing (not the other way around though), which makes the middleware pattern a more critical baseline API to support. I think a blog post showing how to build @jwx 's API on top of the middleware pattern would be an interesting learning resource showing how a declarative model could be built by those that desire it. I'd kick that back to the community to let them build that as a plugin if they want a ready-made, reusable API like that.

jwx commented 5 years ago

I'm really cursing my communication skills here, because I seem to be missing the target all the time. I don't know if it's just me being extremely unclear or also that you're trying to unsuccesfully understand my points through the perspective of the existing spec, but clearly I'm not doing a good enough job expressing my point of view or my ideas.

I'll give it another try, with this specific example, but first let me just point out that my suggestion is just as flexible as the existing spec since everything that can be done with the existing spec can still be done with my suggestion. What my suggestion does is add functionality to the api.

Now, the example: "Add an authCheck hook that's triggered before navigating to all components except Welcome and Login."

According to the spec

The spec api doesn't support adding hooks to only some components on the router level (it does for each route, however, but I'll leave that out now) so we'll present a best practice in the documentation on how to achieve this with the support that the api does provide.

// app.ts (or similar)

const authCheck = (to, from, next): RouteGuard => {
  // adding or removing a route requires declaratively opting a component into auth
  const requiresAuth = to.components.some((component) => component.meta.requiresAuth);
  if (!requiresAuth) { // <--- This is something I added to the example from above
    return next();
  }
  const isAdmin = false;
  if (!isAdmin) {
    return next({ redirect: 'home' });
  }
  return next();
};

router.before([ authCheck ]);

and then in every component except Welcome and Login, something like this

// products.ts
import { route } from '@aurelia/router';

@route(path: 'products', { meta: { requiresAuth: true }}) // <--- My interpretaion of spec. Could also be a static variable
export class Products() { ... }

My suggestion

The above works nicely and would be fully possible with my suggestion as well. The only thing that'd change then is that

router.before([ authCheck ]);

would instead be

router.addBeforeHook([ authCheck ]);

I'm not at all stuck on the name of the method (but I do think it should perform an "add" so that it'd be possible to do from more than one place, but let's not get into that) so if before is the name you want then it'll be identical.

But in my suggestions there's also the possibility to exclude components through an options parameter to the addBeforeHook (or whatever it's called) method. Then, the full example would be

// app.ts (or similar)

const authCheck = (to, from, next): RouteGuard => {
  const isAdmin = false;
  if (!isAdmin) {
    return next({ redirect: 'home' });
  }
  return next();
};

router.addBeforeHook(authCheck, { exclude: [ Welcome, Login ] });

And that's it. There would be no need to for the developer to add their own code for component exclusion or to define meta data on each component that should get an auth check.

Of course, you could always argue that the decision whether the auth check should be performed on a component should be expressed in the actual component code, but that's developer preference. And while it might make sense regarding auth, it might not if we're using hooks to affect animations or component interactions or similar. And my point isn't even where this decision should be expressed, in the products.ts or app.ts (from this example), but that I think the api should provide this feature, to easily exclude components from hooks, and then let the developers decide how they want to structure things.

I agree that api:s should be consistent and not be bloated, but I also think that they should provide a reasonable amount of functionality. Only having router hooks on a subset of the components is a likely use case, and I think the api should support it. Because that's what we're talking about here; whether it's supported by the api or not. It's not two different ways it's supported between my proposal and the spec, it's "here's how it's supported" (my suggestion) compared to "it's not supported by the api, but here's how you can code it yourself" (the spec).

davismj commented 5 years ago

That looks good. On top of this I'd say:

  1. Make include: "*" implicit, i.e. all routes are always included by default. Maybe that was what turned me off about the API the first time.
// fires on all components except Welcome and Login
router.addBeforeHook(authCheck, { exclude: [ WelcomeViewModel, LoginViewModel ] })

// fires only on Test and SomeOther components, 
router.addBeforeHook(timeoutCheck, { include: [ TestViewModel, SomeOtherViewModel ] })

// log a warning or throw an error that says that only include or exclude is supported
router.addBeforeHook(sanityCheck, { include: [ TestViewModel, SomeOtherViewModel ], exclude: [ WelcomeViewModel ] })

// throw an error that says this is broken
router.addBeforeHook(timeoutCheck, { include: [ TestViewModel, SomeOtherViewModel ], exclude: [ TestViewModel ] })
  1. Drop support for magic strings, for now. I could see the benefit of creating some kind of convention, but I still think that would be trying to do too much and would be more of a liability than an asset, from my experience with magic strings.

  2. One remaining difficulty I see is order of execution. If there are three hooks, All, Some, and One. Execution should be Some > One > All. All applies to all components, Some applies to half the components, including SpecialComponent, and One applies to the SpecialComponent. How would you accomplish this?

jwx commented 5 years ago

Yeah, the idea was to have all components included by default. I only added the include: '*' to illustrate the idea I had about wild card capabilities, but you might be right in that magic strings, at least for now, is better left out.

I like the warning/error. Is the second error (broken) needed if the first one (no include/exclude combination) is in place?

As for execution, hooks would have an execution order. I'm thinking defaulting to order of being added, but can be controlled with an executionOrder?: number in options parameter. Execution is then performed one hook at a time that has at least one matching component in the navigation. So in a navigation involving

OrdinaryComponent and SpecialComponent => Some, One, All
OrdinaryComponent => All
SpecialComponent => Some, One, All
davismj commented 5 years ago

I like the warning/error. Is the second error (broken) needed if the first one (no include/exclude combination) is in place?

I'm not sure if both present should be a warning or error. If it's just a warning, then we'd need to throw an error when there's overlap. If it's an error, then it'll be thrown anyway.

executionOrder?: number

I don't think this is a good solution. It's not well defined, as you could have overlapping numbers. It isn't easy, as the developer would have to start tracking the numbers. Last, I don't think you could use that to specify a > b on component one and b > a on component two.

Something to be brainstorming.

jwx commented 5 years ago

Yeah, I haven't really spent enough time thinking about how to achieve that execution order (only what to do when it's executing) to have a good enough suggestion. Some brainstorming definitely needed.

You got a use case where a > b on one component and b > a on another make sense? I see why it could be relevant, but can't come up with an actual example.

EisenbergEffect commented 5 years ago

@jwx I definitely think I had a bit of a misunderstanding. One of the things that's tricky for me is that I have a scope and set of tasks that cover all of the Aurelia ecosystem. So, I'm typically jumping in and out of many very different things really fast. So, in this case, I only had time to look at the latest response or two and provide quick feedback and didn't have time to read the full thread again. That caused me to respond in an awkward way. Your followup post, which included just a little bit more context and code, made things much clearer for me. Thanks!

In any case, I see how the proposal allows for the middleware approach from @davismj and is more about how those hooks get registered. One thing I'd point out here is that we've got some prior art for this in vCurrent. While the middleware isn't as elegant as what's being proposed, we went through a number of iterations on the API to register hooks, create new hooks and order hooks relative to one another. So, it's probably worth looking at that as a starter and making some analysis of what is good and bad about what we did previously to help inform the new API. One thing I think we landed on was a set of explicit APIs for each phase, rather than using string names for phases. We also had a way to create groups of hooks and insert them relative to a known hook's timing, so order wasn't handled with a number but with relativity to fixed points in the lifecycle. I hope this helps provide some ideas there.

fkleuver commented 5 years ago

I definitely agree with the notion of predefined hooks. I think the pipeline slots in the vCurrent router are conceptually nice and effective (just the implementation is messy and we could do this better)

davismj commented 5 years ago

We also had a way to create groups of hooks and insert them relative to a known hook's timing, so order wasn't handled with a number but with relativity to fixed points in the lifecycle.

The problem that I have seen in practice from working with customers is that the majority of developers are only using the auth guard, and some developers are using multiple auth guards. At the moment, order is very clear because they are on the router itself.

router.addAuthorizeStep(Some)
router.addAuthorizeStep(One)
router.addAuthorizeStep(All)

Actually, looking again I think the API is well defined. This would just be:

router.addBeforeHook(Some, { include: [ TestViewModel, SomeOtherViewModel ] })
router.addBeforeHook(One, { include: [ SomeOtherViewModel ] })
router.addBeforeHook(All)

Last, I don't think you could use that to specify a > b on component one and b > a on component two.

router.addBeforeHook(A, { include: [ One ] })
router.addBeforeHook(B, { include: [ One, Two ] })
router.addBeforeHook(A, { include: [ Two ] )

Actually, I think this is nice, well defined, and clear. Maybe a bit verbose, but maybe as succinct as possible for the use case.

The only thing keeping me from full support is the fact that I and many other developers think in routes, and this requires that I know what components are involved in routes. For example, I want a > b on route 'a-first' and b > a on route 'b-first'. How would I solve this using routes?

It's extremely difficult to have any discussion on the router without at least talking about configured routes. We can talk about the conventional route approach, but we need to always have a configured route approach as well.

jwx commented 5 years ago

Since the api supports strings, I was thinking

router.addBeforeHook(A, { include: [ 'a-first' ] })
router.addBeforeHook(B, { include: [ 'a-first', 'b-first' ] })
router.addBeforeHook(A, { include: [ 'b-first' ] )

and that the router knows that these strings are routes and not names of components. (Possibly through a property on the router or similar.)

Regarding the guards: Are there any real use cases that needs to be in a step after the vCurrent auth step? Are the other steps used at all?

davismj commented 5 years ago

I've built some pretty complicated components and I've never needed it in practice. We have a lot of interesting tools for solving problems and I'm usually always able to find a way to avoid digging into internals--which is what the other steps do. What I don't know, and maybe @Vheissu can comment, is whether or not those steps are valuable when composing frameworks. Things like the prerender step.