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

Dynamic routing #122

Merged
merged 11 commits into from
Apr 14, 2020
Merged

Dynamic routing #122

merged 11 commits into from
Apr 14, 2020

Conversation

posva
Copy link
Member

@posva posva commented Jan 27, 2020

Rendered

I wouldn't mind some feedback about the choices made and given alternatives, which is the part I'm the most uncertain of

Questions

If you read the RFC, your feedback on this points would be very valuable:

  • Is it okay to have to explicitly call router.replace to display the new added routes?
  • What would you use getRoutes for?
  • Is it okay to replace the route one already exists with the same name?

@posva posva added router 3.x This RFC only targets 3.0 and above labels Jan 27, 2020
@cawa-93
Copy link

cawa-93 commented Jan 27, 2020

Do you propose the API to register new routes at the component level?

{
  name: 'parent-component',
  
  created() {
    router.addRoute({
      path: '/custom-child-route',
      name: 'custom-child-route',
      component: ChildComponent
    })
  },

  destroyed() {
    router.removeRoute('custom-child-route')
  },
}

@posva
Copy link
Member Author

posva commented Jan 27, 2020

@cawa-93 that should be technically possible based on a loose matching (not matching the end of the path for a route e.g. /some to match /some/nested/path so that the router still manage to render parent components that keep adding routes but you will still need to provide the parent name to addRoute as the router cannot be aware of the context where the function is called

@znck
Copy link
Member

znck commented Jan 29, 2020

@posva Does the addRoute method limit one level of nested routes?

@posva
Copy link
Member Author

posva commented Jan 29, 2020

Does the addRoute method limit one level of nested routes?

No, you can reference a nested route by it's name and add it as a child of that route

@Miljaker
Copy link

When adding a route that has the same name as an existing route, it should replace the existing route and warn about it (in dev only). This is the most convenient version, because it allows replacing new routes without removing the old ones when they are using the same name.

Wouldn't this be extremely annoying if you have a third party component which creates the same route you (or even a different third party component) already used? Maybe increment the route instead and return the incremented route?

@posva
Copy link
Member Author

posva commented Jan 29, 2020

Wouldn't this be extremely annoying if you have a third party component which creates the same route you (or even a different third party component) already used? Maybe increment the route instead and return the incremented route?

I don't know what you mean by incrementing a route. If you mean extend, I think that's bug prone because it can lead to invalid route records.
I think it would be the other way around: it would be very annoying having to remove the route before adding the one that replaces it. But I don't understand what scenario will create over and over the same route without it being destroyed

@Miljaker
Copy link

I don't know what you mean by incrementing a route. If you mean extend, I think that's bug prone because it can lead to invalid route records.

If for example component A adds route: {name: "config", component: 'exA'}. Meanwhile component B adds {name: "config", component: 'exB'}. We wouldn't be able to get to exA anymore because it got replaced right?
This seems extremely annoying to me because you wouldn't want to touch third party components simply to change the name of the route it creates.
So what if instead of replacing it, the second one automatically gets renamed to "config2"? And then obviously return this renamed name so the component can use it in it's links.

To be honest I was just thinking out loud (thinking out writing?). I can think of plenty of other issues with this.

@posva
Copy link
Member Author

posva commented Jan 29, 2020

it sounds to me that you want to add new routes that are effective without caring about the name. In that case, just omit the name and use the callback returned to remove the route whenever needed. A symbol could also be used to ensure unique identifiers, specially when dealing with nested routes

So what if instead of replacing it, the second one automatically gets renamed to "config2"? And then obviously return this renamed name so the component can use it in it's links.

I don't think that's good, it's implicit and doesn't look intuitive for most cases. It's either replace existing route record or fail and throw an error explaining why

@donnysim
Copy link

Personally would prefer an option to specify if we want to replace an existing route, because most of the cases replacing it won't be intended and getting an error would be preffered. Maybe a replace function that adds if not exists or replaces.

There might be cases where we would want to specify a position where the route is added, e.g we want to add /users/all before /users/:id route.

For libraries, they should add routes with a prefix because nobody wants to deal with route renaming when someone thought it's a good idea to use a common route name in a library. Or they should at least provide options for route naming, so I don't think this should concern the router.

Also, would we be able to add a route without url? most cases are the wrapper layout component, where we currently have to assign an empty url and then specify the real urls in the childs, yet it's not guaranteed to contain a root route, so we also have to write a guard on the wrapper to redirect to first child route if we hit the parent url.

Any thoughts on it?

@posva
Copy link
Member Author

posva commented Jan 29, 2020

because most of the cases replacing it won't be intended

I've experienced the opposite. In what scenario would you use dynamic routing?

There might be cases where we would want to specify a position where the route is added, e.g we want to add /users/all before /users/:id route.

I didn't mention it but ordering is no longer a concern, Vue Router is able to know what order to use. Proper dynamic routing wouldn't be possible without that

For libraries, they should add routes with a prefix because nobody wants to deal with route renaming when someone thought it's a good idea to use a common route name in a library. Or they should at least provide options for route naming, so I don't think this should concern the router.

I don't understand where this comes from. Why would a library add a route in the first place? Why would they even use a common name if it's a library?

Also, would we be able to add a route without url? most cases are the wrapper layout component, where we currently have to assign an empty url and then specify the real urls in the childs, yet it's not guaranteed to contain a root route, so we also have to write a guard on the wrapper to redirect to first child route if we hit the parent url.

No, a route needs a url. The layout idea is beyond this RFC

@znck
Copy link
Member

znck commented Jan 29, 2020

Can symbol be used as a route name?

@donnysim
Copy link

I've experienced the opposite. In what scenario would you use dynamic routing?

At the moment we have a "modular" structure, where we drop in a module and webpack picks up the module bootstrap js file which registers routes, stores etc. so the least we want is to debug which of the 25+ modules overwrote a route, sometimes because someone forgot to change the name if copied. Some sort of indication that a route was rewritten would be nice to have.

I don't understand where this comes from. Why would a library add a route in the first place? Why would they even use a common name if it's a library?

That's kind of just a response to the previous comment:

This seems extremely annoying to me because you wouldn't want to touch third party components simply to change the name of the route it creates.


Is a missing route in scope of this RFC? Like what happens when a route is not found and how do we handle it?

@posva
Copy link
Member Author

posva commented Jan 30, 2020

Can symbol be used as a route name?

I think that the implementation cost is zero as it's more of a typing thing, meaning that it wouldn't change anything for IE users. and it would make sense when adding routes by the user where we cannot know the name.

the least we want is to debug which of the 25+ modules overwrote a route, because someone forgot to change the name if copied

How do people decide to add routes and name them? Is it just due to human nature and making mistakes? In that case we could have a method getRoute(name: string | Symbol) to do that kind of checks (this is considering replacing routes is more often the desired behavior)

Do you have a link to the library you are building with some description about it? It might help me understand why a library would add routes to an application that also handles routes on its own

Is a missing route in scope of this RFC? Like what happens when a route is not found and how do we handle it?

What method are you talking about? I think that when removing, if the route doesn't exist, it should warn in dev. When adding children route, it should throw an error if the parent doesn't exist (this needs to be added to the RFC)

@donnysim
Copy link

How do people decide to add routes and name them? Is it just due to human nature and making mistakes? In that case we could have a method getRoute(name: string | Symbol) to do that kind of checks (this is considering replacing routes is more often the desired behavior)

It is more of a human nature, e.g.

export default function () {
  Router.addProtectedRoute({
    name: `users.index`,
    path: 'users',
    component: () => import(/* webpackChunkName: 'users' */ './Index'),
  });
}

and then someone copies it for employees etc. and forgets to change the name from users to employees. Though this could be solved with the getRoute function and doing the check in user land with router wrapper so I guess not a big deal.

Do you have a link to the library you are building with some description about it? It might help me understand why a library would add routes to an application that also handles routes on its own

Personally never seen a library that adds it's own route, maybe if it's something internal, unless we could call our modules as internal libraries, but they're still tied to the overall project setup. Can't really imagine any public library adding a route (because each project has it's own layout, styles etc.). I don't think this was ever a question so maybe I shouldn't have poked it. The only thing I could think of maybe a page adds a modal "route" on enter, where on any page you could trigger a modal by child route but that also feels far stretched.

What method are you talking about? I think that when removing, if the route doesn't exist, it should warn in dev. When adding children route, it should throw an error if the parent doesn't exist (this needs to be added to the RFC)

More on the line of how do we handle 404 page. Or what happens when you remove currently active (opened) route (maybe some setup reloads all routes based on permissions or user role, or maybe generates it)? though on that note there is not clear all routes method so maybe not an option.

@posva
Copy link
Member Author

posva commented Jan 30, 2020

Thanks for the details. I wanted to make sure the API is still flexible enough and it seems like something like hasRoute will help you

More on the line of how do we handle 404 page. Or what happens when you remove currently active (opened) route (maybe some setup reloads all routes based on permissions or user role, or maybe generates it)? though on that note there is not clear all routes method so maybe not an option.

This RFC does not affect 404. When you remove the currently active route, the router reroutes, you stay on the same page but you can call push/replace to trigger a new navigation (example added). I can add more details to that part on the RFC but in big lines, it's like a regular navigation that replaces current location instead of pushing. Same rules for navigation guards apply.

@michaeldrotar
Copy link

I think an alternative would be to just compose an object.. and then create a router.

const routes = [];
routes.push({ path: '/user/:id', component: User });
routes.push({ path: '/about', component: About });
const router = new VueRouter({ routes });

If 3rd parties want to provide routes...

import { externalRoutes } from 'external';
const routes = [].concat(externalRoutes);

And the 3rd party routes could be modified...

import { externalRoutes } from 'external';
const modifiedExternalRoutes = externalRoutes.map((route) => {
  route.path = `/external/${route.path}`;
});
const routes = [].concat(modifiedExternalRoutes);

Or perhaps just use the components and not the routes...

import { externalComponent } from 'external';
const routes = [];
routes.push({ path: '/external/page', component: externalComponent });

I'm not sure I understand the use case though beyond 3rd party stuff...

@posva
Copy link
Member Author

posva commented Jan 30, 2020

@michaeldrotar The idea of dynamic routing is to not recreate a router instance (https://github.com/posva/rfcs/blob/router/dynamic-routing/active-rfcs/0000-router-dynamic-routing.md#motivation)

Making a property like routes reactive will either be incomplete (can only add/remove entries to the array) or create many edge cases (modifying an existing route) and in my experience, we usually create routes instead of modifying existing one. On top of that, it's still possible to create a new route using an existing route but overriding some parts of it

@rijkvanzanten
Copy link

@posva What's the reason to rely on name instead of path for removeRoute? Isn't path both required and unique?


In your alternative design for addRoute, how would Vue Router match the route when passing that to removeRoute? If the unique name is missing, how would it know which route to remove?

const normalizedRouteRecord = router.addRoute(routeRecord)
router.removeRoute(normalizedRouteRecord)

Regarding the listed drawbacks: did you leave the instructions in? Starting from "Why should we not do this?" it looks unrelated to the specific RFC.

@posva
Copy link
Member Author

posva commented Feb 22, 2020

What's the reason to rely on name instead of path for removeRoute? Isn't path both required and unique?

name is unique by design but path isn't. Finding by path only work for simple paths. I don't see anybody writing router.removeRoute('/some/nested/url-:with-:id([0-7]{3,4})'). It also breaks if the path changes

In your alternative design for addRoute, how would Vue Router match the route when passing that to removeRoute?

By reference

If the unique name is missing, how would it know which route to remove?

By using the returned function (there is an example on the RFC)

I forgot to remove a few items on the list in Drawbacks indeed!

@posva
Copy link
Member Author

posva commented Feb 27, 2020

I added a few updates, namely the router no longer rerouting after adding/removing a route. While implementing and experimenting with the feature on v4, I realized automatically rerouting is not flexible enough because we cannot wait for the navigation to be finished. removing that behavior is lighter and allows the user to have full control over how the navigation happens

@muthu32
Copy link

muthu32 commented Mar 11, 2020

I still like react way of dynamic routing.
https://reacttraining.com/react-router/web/example/nesting
Can it be implemented in Vue using components?
Like Adding and removing the router based on the existing component list.

@aztalbot
Copy link

aztalbot commented Mar 11, 2020

@muthu32 have you looked at this project: https://github.com/blocka/vue-component-router ?

I suspect this dynamic routing rfc might enable something similar but I’m not sure that’s the intent. I think it’s more to address certain use cases where dynamic routing would be useful (which I think is fantastic) rather than always doing dynamic routing vs static routing.

@posva
Copy link
Member Author

posva commented Mar 11, 2020

Dynamic routing does enable declarative routing (what react router has) but it won’t be implemented directly by Vue router, at least yet. This RFC should provide an api that allows implementing declarative routing

@posva
Copy link
Member Author

posva commented Apr 7, 2020

This RFC is now in final comments stage. An RFC in final comments stage means that:

The core team has reviewed the feedback and reached consensus about the general direction of the RFC and believe that this RFC is a worthwhile addition to the framework.
Final comments stage does not mean the RFC's design details are final - we may still tweak the details as we implement it and discover new technical insights or constraints. It may even be further adjusted based on user feedback after it lands in an alpha/beta release.
If no major objections with solid supporting arguments have been presented after a week, the RFC will be merged and become an active RFC.

@posva posva added the final comments This RFC is in final comments period label Apr 7, 2020
@posva posva merged commit 1d49580 into vuejs:master Apr 14, 2020
@posva posva deleted the router/dynamic-routing branch April 14, 2020 11:44
@shijunti19
Copy link

shijunti19 commented Jul 20, 2020

@posva
I like this set of methods and code

let routed=new route();
routed->path('/root')->component(Component)->name('root')->children(routed=>{
   routed->path('/root')->component(Component)->name('root');
});
routed->path('/root')->component(Component)->name('root')->param(routed=>{
   routed->title='title';
routed->icon='list';
routed->url='/root/add';
routed->param={id:1}';
});
routed->rend();
let menus=routed->menus();//Build array user generated menu
let menus=routed->list();//Show full route

@lefreet
Copy link

lefreet commented Jul 23, 2020

there some way to reset routes?in my way to hack it now:

router.matcher = new router.constructor(router.options).matcher

expected

router.cleanRoutes() // remove all routes, with options.routes
router.resetRoutes() // remove dynamic routes, keep options.routes

@posva
Copy link
Member Author

posva commented Jul 23, 2020

we could make it so we can write

router.getRoutes().forEach(router.removeRoute)

@XingXiaoWu
Copy link

I added a few updates, namely the router no longer rerouting after adding/removing a route. While implementing and experimenting with the feature on v4, I realized automatically rerouting is not flexible enough because we cannot wait for the navigation to be finished. removing that behavior is lighter and allows the user to have full control over how the navigation happens

So now, how to initiate a new navigation after the dynamic routing operation? I see that the documentation does not recommend using next anymore

@posva
Copy link
Member Author

posva commented Apr 29, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x This RFC only targets 3.0 and above final comments This RFC is in final comments period router
Projects
None yet
Development

Successfully merging this pull request may close these issues.