Working out on routing

[arn-the-long-beard]

Hello everybody 😃

UPDATE

Here is the latest API

/// The RoutingModule makes the enum variants representing modules loaded by the routes
/// By default, an enum variant snake case is equal to its module name
///
///  You can rename the path
///  You can specify routes that does not load module ( no init, no specific Model & Msg and no view )
///
///  When loading the module, a parser will get the init function , Model, Msg, Routes, Update, and View
///

#[derive(Debug, PartialEq, Clone, RoutingModules)]
// need to make a derive (Routing) or something maybe
#[modules_path = "pages"]
pub enum Routes {
    Login {
        query: IndexMap<String, String>, // -> http://localhost:8000/login?name=JohnDoe
    },
    Register,
    #[guard = " => guard => forbidden"]
    Dashboard(DashboardRoutes), // -> http://localhost:8000/dashboard/*
    // // #[default_route]
    #[guard = " => admin_guard => forbidden_user"]
    Admin {
        // -> /admin/:id/*
        id: String,
        children: AdminRoutes,
    },
    #[default_route]
    #[view = " => not_found"] // -> http://localhost:8000/not_found*
    NotFound,
    #[view = " => forbidden"] // -> http://localhost:8000/forbidden*
    Forbidden,
    #[as_path = ""]
    #[view = "theme => home"] // -> http://localhost:8000/
    Home,
}

I previously had OnInit and OnView derive but I removed them since RoutingModules is enough. I also had #[model_scope] for fn init that I removed since it is automatically done with the RoutingModules now

End of Update

This PR is related to #383

I am soo happy to show you what I have done so far but on the other hand, I am not entirely sure about your expectations regarding routing in general. This is just an example and the PR is a draft. This routing is built on the top for what we already have in Seed.

Basically my router listen to Msg::UrlRequested and then do stuff

The router can also accept closure to notify.

|r| orders.notify(subs::UrlRequested::new(r))

All your inputs are welcome to higher up this example to match your expectations & standards. As a newbie to rust I am facing few challenges:

• I have never implemented something like this from scratch (in rust at least).
• My main strength in Rust is doc and test and finding bug.
• I do not understand macro stuff for now.
• I have never been this deep into routing at least this year.
• I have some issue with enum.

I want to help and at the same time challenge myself, that is why I accepted starting something on routing also I did not look at any tutorials.

Here is the todo list on this example that might be relevant.

• add UML schema that show the lifecyle of the router.
• navigate through route, update url , go back & forward in history as mentioned in the issue.
• having authenticated routes and guard them.
• having nested routes.
• having unit test.
• work on the api.

PS: Here is the new todo list 👍
There are a lot of things probably wrong right now in the implementation, but it does the job for simple routing.

PS: my boilerplate in the init is ugly 💩 , we need to fix it 😝

Let's see what you think 👍 and let's make it beautiful ❤️

Update

We are working with @mankinskin together on it.

Now we have a crate for parsing enums into path : enum_paths

So we moved further into solving the nested route challenge:

• Nested routing now works
• Presenting the route needs to be done

So here is an intermediate todo that I suggest

• change attribute "name" to path.

#[derive(Debug, PartialEq, Copy, Clone, AsPath)]
pub enum DashboardRoutes {
    Message,
    Statistics,
    #[path  = ""]
    Root,
}

• find a way to get the name :

ex : the route "http://localhost:8000/dashboard/message" would be Message

let name = Routes::Dashboard(DashboardRoutes::Message).get_name();
log!(name);
//--> Message

• find a way to display routes & nested routes in a menu.

├── Home
├── Login
├── Register

└── Dashboard
         ├── Root
         ├── Message
         ├── Statistics
├── NotFound

I need to chew on it tonight but I think I am having an idea on what to do tomorrow :

• Implement extracting query parameter from String to enum and from enum to String + derive Macro

"dashboard/tasks/task/1?assigned=arn-the-long-beard" 

• Implement nested routes inside dynamic + derive Macro if needed

"dashboard/tasks/task/1/description"

• Handle default route when no routes are matched + derive Macro if needed

pub enum MyRoutes {
    Login,
    Register,
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""]
    Home,
    // Admin(page::admin::Model),
}

• Handle protected route + derive Macro if needed
  How to specify a route is guarded ? custom attributes or specific field ?

• Give access on the routing for init of page/component + derive Macro if needed

Msg::UrlChanged(subs::UrlChanged(url)) => model.page = Page::init(url, orders),

We need this to control init and pass orders to sub pages/components

• Implement conversion to Route struct which hold information + impl + derive Macro if needed

pub struct Route {
  pub full_path: String,
    pub url: Option<Url>,
    pub segment:String,
    pub guarded: bool,
    pub default: bool,
    pub query_parameters: HashMap<String, String>,
    pub value: Routes,

Having this live conversion could help us to manipulate routes inside sub_pages / component easier

• Unify the Api for custom router
  Having one single Router generated by the enum maybe
  Having easy API this way

#[derive(Routing)]
pub enum Routes {
    Login,
    Register,
    #[protected_route="user"]
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""]
    Home,
    // Admin(page::admin::Model),
}

#[derive(Routing)]
pub enum Routes {
    Login,
    #[init = "pages::register::init"]
    Register,
    #[protected_route="user"]
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""],
    Profile { param_id : String }, 
    Home { query_parameters : Hashmap<String,String> },
    // Admin(page::admin::Model),
}

[mankinskin]

Hi @arn-the-long-beard , I looked at your code yesterday, and honestly I think it is kind of doing too much. I could not really get a grip on what it should do. Could you maybe write up a list of things you want the router to do? You said earlier that you think it should be refactored, and I think the first step is always to find clear goals you want your code to accomplish.

Personally my checklist for the routing looks something like this:

• convert nested Route enums to String paths ("/users/user/1")
• parse Route enums from String paths (Route::Users(UserRoute::User(1)))
• derive macro for implementing this (strum basically does this)
• add protocol, hostname and port to seed::URL

There might be some things missing, and I would be glad if you could add them to the list. I guess there is another topic of handling navigation with routes, but I am not sure of everything there is that to be handled, so I just am just focussing on mapping enum Routes and path strings for now.

[mankinskin]

Hi, I just released the enum_paths crate, which might make converting enum routes between strings easier :) maybe you can use it?

[arn-the-long-beard]

Hello !!!!

Thank you so much for your help !

I just have looked at your enum_paths, this is exactly what I needed and tried to accomplish. I am taking a look at your awesome code first to see how it is working because I need to understand 😋 and then I will use it !

Thank you so much !

By the way, I mostly code during the week and I do a break during week end, so I could not answer much.

[arn-the-long-beard]

Okay, I think I understand most of the code and I see how you made it recursive so nested routes are converted 😃

Your code is easy and very structured and easier to read than mine. We can see where you are going.

My level in Rust is far bellow yours and I really do need mentorship to progress.

There is one thing so that we need to checkout. We need to remove the need for using Nightly build. I think we should not use nightly build even for example in Seed because we should always be in stable 💯 on Seed master.

I have been trying to understand where the need for the the nightly build come from and I get this message

use of unstable library feature 'bool_to_option'

Is there a way to know which library is triggering the feature and if there is a way to avoid it ?

I need to re think my router and come up with a better plan.

Thank you again for your help and for challenging me 👍

[mankinskin]

Always glad to be able to help 😄 I just pushed the removal of the nightly code 👍

I think the best ways to improve your Rust skills are writing code yourself and reading other peoples code. Pure exposure. That way you get the right ideas quicker, because you see what other people came up with. So you are already on the right path 🙂

And apart from learning the language itself, a big part is also to have a clear vision of your goal. In my experience, the workflow goes from an intuitive idea, over a declarative set of requirements to the imperative code. So you start out with some problems you want to solve and get a rough idea how they could be solved. Then you break that down into as simple problems as possible, and declare what a solution would look like. Then you have the requirements and start designing code that solves each of these problems individually.

By now I know the difference between when I am just coding without knowing where I am going and coding to implement a clear vision that I already came up with. The latter is much more efficient and produces better code.

This was an easy example, because it is only one problem to solve really. The router is of course much bigger and more complex, and I am even unsure what exactly is the desired behavior. But defining that would be the first step obviously.

[arn-the-long-beard]

Good morning @mankinskin

Thank you so much for your input 😄

Yes I am trying to have clear goals. I think I just did not break down the router into small steps enough.

[arn-the-long-beard]

Hello guys.
@mankinskin , thank you for your crate !

I started to use enum_paths and I got some ideas for future features in it 😄

Your crate helps me so much to think simpler 👍 . I will also see the code made by @TatriX as well and use it if he is fine with it 😄

For now the challenge is when I try to iterate on the routes. I have a Default on nested Enum, so I do not get all the routes as you can see in the unit test which is failing.

Tomorrow I will code as well and rewrite the plan for this PR.

Question about Github: Can many people collaborate on the same PR ? Can somebody fork my Forked repos and then commit in the same PR ?

[mankinskin]

Can many people collaborate on the same PR ? Can somebody fork my Forked repos and then commit in the same PR ?

I have never done this, but it should be possible to fork your fork and then create a PR for your fork which can then be merged into this PR. it's a bit of a hassle but should work.

[mankinskin]

I am trying to implement a router for my app right now, but it is still very unstable in a lot of places. here you can find the code.

For example, when I call router.go_to(Route) upon receiving either UrlRequested or UrlChanged, it gets called twice, because UrlRequested also causes a UrlChanged event. Probably we should drop UrlRequested entirely and just change the browser url instead when clicking a link and then send a UrlChanged message.

[arn-the-long-beard]

Okay, I can help you with that because I solved this with the custom_router. Le mer. 30 sept. 2020 à 21:48, Linus Behrbohm <[email protected]> a écrit :

…

I am trying to implement a router for my app right now, but it is still very unstable in a lot of places. here <https://github.com/mankinskin/binance-bot/blob/master/src/client/router/mod.rs> you can find the code. For example, when I call router.go_to(Route) upon receiving either UrlRequested or UrlChanged, it gets called twice, because UrlRequested also causes a UrlChanged event. Probably we should drop UrlRequested entirely and just change the browser url instead when clicking a link and then send a UrlChanged message. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHFYCCN53GIJLSKK7SY2M2DSIODP5ANCNFSM4RE2GYKQ> .

[mankinskin]

How did you solve it? I am just not responding to UrlRequested messages at all in the router now.

[arn-the-long-beard]

Good morning ☕

Here are the explanation from my tests and observations :

• In Seed when you click on a link like At::Href => route.url It will trigger one UrlRequested and one UrlChanged everytime. As Martin said on discord, it allow us to intercept stuff in UrlRequested for example.

• I do connect my router from changes from UrlChanged, not from UrlRequested for now.

• I do use orders.notify(subs::UrlRequested::new(r)) called from my app, not from the router itself , then it will start the Url Cycle and goes to UrlRequested -> UrlChanged -> Then updating the routing . This is not the way it should be in the future maybe, but here we are building an example on top of Seed.

• I make sure to have control of the life cycle in one place to have the overview : the root update function of my App in lib.rs

Here is the update on my example App

fn update(msg: Msg, model: &mut Model, orders: &mut impl Orders<Msg>) {
    match msg {
        Msg::UrlChanged(subs::UrlChanged(url)) => {
            log!("URL has changed");
            model.router.confirm_navigation(url);
            // model.page = Route::init(url);
        }
        Msg::UrlRequested(request) => {
            log!("URL requested");

            // let url = request.0;
        }
        // Stuff in between
        Msg::GoBack => {
            model
                .router
                .request_moving_back(|r| orders.notify(subs::UrlRequested::new(r)));
        }
        Msg::GoForward => {
            model
                .router
                .request_moving_forward(|r| orders.notify(subs::UrlRequested::new(r)));
        }
    }
}

I am gonna take a look at your router and see.

[arn-the-long-beard]

You have nice code 😃
Can you show me how you were listening to UrlChanged ?

[arn-the-long-beard]

Okay, I switched to SuperRouter, which is the router revamped to use enum_paths.

Here are the points I can Observe by using enum_paths:

1 - Easier to make routes from the user perspective :

• Less imports.
• Less attributes.
• Easy to connect nested routes together.
• Nested navigation does actually works 👍 ( it was not working before ) .
• Hard to iterates on the routes and I do not know yet how to display them.
• We are missing name/title for the route ex : a route "" can be named Home for example. But for now we only have information about the path of the route , not its name.

I would like something like this as a navigation menu

├── Home
├── Login
├── Register

└── Dashboard
         ├── Root
         ├── Message
         ├── Statistics
├── NotFound

2 - For the developer of the router.

• Easy to use but a bit more writing than before ( I had a hashmap and few shortcuts from strum ).
• Maintenance is easier since no arrays and no hashmap and so less dependencies.
• Easy to contribute on enum_paths and add new feature to it.
• Much less dependencies.
• It is working well for the nested perspective but hard to Iterate while before I could iterate but it was not working.

Navigation is working same as before for standard go, back and forward as well as which route is active.

I think we are going somewhere. I will edit the PR description and update with a new todolist 😄

[arn-the-long-beard]

I updated the PR first comment with this

Update

We are working with @mankinskin together on it.

Now we have a crate for parsing enums into path : enum_paths

So we moved further into solving the nested route challenge:

• Nested routing now works
• Presenting the route needs to be done

So here is an intermediate todo that I suggest

• change attribute "name" to path.

#[derive(Debug, PartialEq, Copy, Clone, AsPath)]
pub enum DashboardRoutes {
    Message,
    Statistics,
    #[path  = ""]
    Root,
}

• find a way to get the name :

ex : the route "http://localhost:8000/dashboard/message" would be Message

let name = Routes::Dashboard(DashboardRoutes::Message).get_name();
log!(name);
//--> Message

• find a way to display routes & nested routes in a menu.

├── Home
├── Login
├── Register

└── Dashboard
         ├── Root
         ├── Message
         ├── Statistics
├── NotFound

[mankinskin]

Can you show me how you were listening to UrlChanged ?

That is happening using seeds subscribe API, see here and this seed example. Note that there is orders.subscribe(..) and orders.subscribe_with_handle(..) with the handle letting you control the lifetime of the subscription. I had to use that so I can reconstruct components over and over without accumulating subscriptions.

[mankinskin]

Very interesting ideas. Yes it would be very useful to give routes individual titles. However I think this could be easily realized using a ToString implementation for Route. Because this doesn't require any nesting, there is not much boilerplate involved:

impl ToString for Route {
    fn to_string(&self) -> String {
        match self {
            Route::Home => "Home",
            Route::Auth(auth_route) => &auth_route.to_string(),
            Route::Chart => "Chart",
        }.to_string()
    }
}

[mankinskin]

find a way to display routes & nested routes in a menu.

This is indeed a nice idea. One could probably write a macro to generate this. I would start by writing the code by hand and finding the spots that need to be programmed by a macro. Arguments the use should be able to set, parts of the code the macro should read, and repeating patterns the macro should generate.

I think it would be nice if the user could decide what kind of elements should be used to represent the links. The Href path can be gotten from the AsPath implementation, the Display name can be gotten from ToString or Display.

By now I have always build navbars and links like this:

div![

    a!["Home", attrs!{ At::Href => "/" }],
    a!["Chart", attrs!{ At::Href => "/chart" }],
    a!["Login", attrs!{ At::Href => "/login" }],
    a!["Register", attrs!{ At::Href => "/register" }],

    self.page.view().map_msg(Msg::Page)
]

I guess nesting requires a bit more logic, and we probably want optional expanders aswell. I think we should start with generating the above for one level of routes. This is actually a navigation tree/bar for these routes:

enum Route {
    Chart,
    #[name = ""]
    Auth(AuthRoute),
    #[name = ""]
    Home,
}
enum AuthRoute {
    Login,
    Register,
}

So there is one level of nesting to make implementation easier, but it isn't reflected in the route paths at all, and should not be noticable. So we need a way to handle that aswell.

[arn-the-long-beard]

Good morning ☕

This is indeed a nice idea. One could probably write a macro to generate this. I would start by writing the code by hand and finding the spots that need to be programmed by a macro. Arguments the use should be able to set, parts of the code the macro should read, and repeating patterns the macro should generate.

I think it would be nice if the user could decide what kind of elements should be used to represent the links. The Href path can be gotten from the AsPath implementation, the Display name can be gotten from ToString or Display.

Yes, there is already the code in this PR for that but my macro code is not as nice as your. I will rewrite it in a more friendly version 👍

[arn-the-long-beard]

impl ToString for Route {
    fn to_string(&self) -> String {
        match self {
            Route::Home => "Home",
            Route::Auth(auth_route) => &auth_route.to_string(),
            Route::Chart => "Chart",
        }.to_string()
    }
}

Actually we should get these names :

• Home
• Auth
• Chart

I have it to work with strum right now in snake_case because I asked to serialize as snake_case

    #[test]
    fn test_names() {
        let name = ExampleRoutes::Dashboard {
            children: DashboardRoutes::Root,
        }
        .to_string();

        assert_eq!(name, "dashboard");
    }

[arn-the-long-beard]

Okay here is the result after I implemented name for routes.

[arn-the-long-beard]

Today I used enum_path and the SuperRoute to display many more nested routes and here is the result

The result is that there is a lot of code to write actually.

If I use the init way same as here https://github.com/MartinKavik/seed-app-time-tracker/blob/master/src/lib.rs#L159 , it could be a lot of code especially for the sub routes.

If I compare to Angular routing :

When we use the route with this custom router, we need to specify the entire path ex : /dashboard/tasks/task/1 to build the url like this while in angular, we can use the relative url.

As well, we need also to use the complete Enum this way

   let task_url = SuperRouter::<Routes>::url_static(&Routes::Dashboard(DashboardRoutes::Tasks(
        TasksRoutes::Task(task.task_no),
    )));

Also the access to the Router from a subpage is difficult since we need to inject it in the view.

There is no need for this in Angular. From what I remember, just using Dependency Injection and call the Router in a constructor and then we can get what we need. But of course, in Seed we do not work with component.

Maybe we can find a way to call the Router from anywhere without the need to have it in view so we can handle navigation in easier way.

These observation are subjective because it come from my personal experience.

[arn-the-long-beard]

I think my focus on getting a Name come from the difficulties to access a specific route from inside the code I guess.

Here is some angular code to display a navigation item with a specific class when active

   <li [routerLink]="['/task/'+id]"  routerLinkActive="active-link">
        <button mat-button>Task {{id}}</button>
        </li>

I am going to see if I can come up with something to make the writing of routes easier inside html.

[mankinskin]

With enum_paths you should be able to get the path of a sub route easily.. if you have a nested enum you can just call as_path on the sub enums.

#[derive(AsPath)]
enum Route {
    Top(SubRoute),
}
#[derive(AsPath)]
enum SubRoute {
    Sub
}
Route::Top(SubRoute::Sub).as_path()
// /top/sub
SubRoute::Sub.as_path()
// /sub

[arn-the-long-beard]

Yes, this works nicely 👍

[arn-the-long-beard]

Okay, today I have been looking into 2 router implementation :

• @mankinskin 's router
• @TatriX 's router

They both have good stuff to get inspiration from that I can add to this custom router.

For now the custom router has :

• history
• can go back and forward
• know which url is active
• support nested routes as long as the path is not dynamic

I need to chew on it tonight but I think I am having an idea on what to do tomorrow :

• Implement extracting query parameter from String to enum and from enum to String + derive Macro

"dashboard/tasks/task/1?assigned=arn-the-long-beard" 

• Implement nested routes inside dynamic + derive Macro if needed

"dashboard/tasks/task/1/description"

• Handle default route when no routes are matched + derive Macro if needed

pub enum MyRoutes {
    Login,
    Register,
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""]
    Home,
    // Admin(page::admin::Model),
}

• Handle protected route + derive Macro if needed
  How to specify a route is guarded ? custom attributes or specific field ?

• Give access on the routing for init of page/component + derive Macro if needed

Msg::UrlChanged(subs::UrlChanged(url)) => model.page = Page::init(url, orders),

We need this to control init and pass orders to sub pages/components

• Implement conversion to Route struct which hold information + impl + derive Macro if needed

pub struct Route {
  pub full_path: String,
    pub url: Option<Url>,
    pub segment:String,
    pub guarded: bool,
    pub default: bool,
    pub query_parameters: HashMap<String, String>,
    pub value: Routes,

Having this live conversion could help us to manipulate routes inside sub_pages / component easier

• Unify the Api for custom router

Having one single Router generated by the enum maybe
Having easy API this way

#[derive(Routing)]
pub enum Routes {
    Login,
    Register,
    #[protected_route="user"]
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""]
    Home,
    // Admin(page::admin::Model),
}

#[derive(Routing)]
pub enum Routes {
    Login,
    #[init = "pages::register::init"]
    Register,
    #[protected_route="user"]
    Dashboard(DashboardRoutes),
    #[default_route]
    NotFound,
    #[as_path = ""],
    Profile { param_id : String }, 
    Home { query_parameters : Hashmap<String,String> },
    // Admin(page::admin::Model),
}

Then when this is done. I will just complete the different pages and I ll push the PR and we can start playing the real implementation inside Seed and start working as @mankinskin suggested by using the feedback from this case.

[mankinskin]

what I would do is move only the minimal required code into a new crate and while doing this, it would be nice if you could find smaller packages which can later be reused by other crates, and move them into their own crates aswell (kind of like enum_paths).

It seems like this crate contains a lot of business logic, which would be overwhelming for an example of a router, and should probably go into separate examples. So I think the main part right now is the router module and two simple pages from the pages module. Everything regarding authentication should be part of another example.

Sorry, but I am still not exactly clear what you expect a "router" to do. Because I think you can get all of the history navigation done by the browser.. when you use the browser navigation buttons in a Seed app, UrlChanged is fired. Any component can listen to this and react. What I understand is that you want to emulate these controls in the seed app. But I don't really see the point of doing that. But maybe I am unaware of some use-cases for this.

[mankinskin]

And if the router should be merged into seed as an integrated routing component, then I would suggest to move it into its own crate completely and have the example import it. That way it will be easier to separate which code is actually part of the router, and which is the code using it in the example. Then it will also be easier to test the router and see what the current API is it exposes.

[MartinKavik]

Should we review the PR and merge?, or put the code in a specific crate ( as @mankinskin suggested before) and test it, improve it and then merge it later when more complete ?

I agree with @mankinskin - a new crate/repo would be the most reasonable option.

Some reasons why I wouldn't want to merge it in the current state (after a quick scrolling through the changes):

• Too much code to review at once.
• No entries in the CHANGELOG.md - what changes are there? (Especially the breaking ones.)
• Why I see my old changes (e.g. in BACKERS.md)? I looks like there were some problems during the merge.
• I can (and want to) only rebase changes onto master - it means that your commit history should be clean and nice (i.e. squashed commits, no commits like "Merge branch.." and your branch should be rebased on the current Seed's master).
• I see some commented code in the PR.
• I don't see any docs comments.
• CI failed.
• We would have to update all seed-rs.org tutorials (docs + source code) if there are breaking changes.
• I would be very surprised if everything would work without bugs and API would be polished enough to use in production apps immediately after the merge. Bug fixes/updates would make Seed less stable and we would have to release new Seed versions too often.
• The new router isn't integrated into the older examples in the Seed repo,
• I think reviewers would need to read at least a simpler description how the new router works to speed up the review process a lot.
• How the new code affects compilation time, WASM file size, CI time, etc.?
• It looks that the PR contains a new example, new router and Seed changes without clear boundaries. The directory/module structure or at least commits should divide changes to logic chunks.
• etc.

So I would suggest steps like these:

1. Extract the router and the new example into a standalone repo (we can move it under seed-rs Github org if you want).
   • Probably there are some Seed parts that we will need to modify to make the extraction possible. We would discuss them individually.
2. Write more examples (including some really simple ones).
3. Write docs - at least reasonable README.md + description for examples.
4. Improve API and fix bugs until we'll get confident enough that it's stable and users like it.
5. Decide if we should keep it the standalone repo or move to Seed or move partially (e.g. only improve Url or something like that).
6. Write official docs, docs comments, etc. specific for the router.
7. Merge / move to Seed / release a standalone seed-router crate.
8. Update seed-rs.org tutorials and Seed examples as necessary.
9. Release a new Seed version.

[arn-the-long-beard]

Thank you guys for your messages. I will read them more tomorrow. But this PR is only supposed to add an example in /examples. I am not changing anything into seed/src. So there is no need to change the tutorials or changelog. I think we have à confusion. 😁 There is definitly an issue with the rebase I did. Le mar. 27 oct. 2020 à 17:53, Martin Kavík <[email protected]> a écrit :

…

Should we review the PR and merge?, or put the code in a specific crate ( as @mankinskin <https://github.com/mankinskin> suggested before) and test it, improve it and then merge it later when more complete ? I agree with @mankinskin <https://github.com/mankinskin> - a new crate/repo would be the most reasonable option. Some reasons why I wouldn't want to merge it in the current state (after a quick scrolling through the changes): - Too much code to review at once. - No entries in the CHANGELOG.md - what changes are there? (Especially the breaking ones.) - Why I see my old changes (e.g. in BACKERS.md)? I looks like there were some problems during the merge. - I can (and want to) only rebase changes onto master - it means that your commit history should be clean and nice (i.e. squashed commits, no commits like "Merge branch.." and your branch should be rebased on the current Seed's master). - I see some commented code in the PR. - I don't see any docs comments. - CI failed. - We would have to update all seed-rs.org tutorials (docs + source code) if there are breaking changes. - I would be very surprised if everything would work without bugs and API would be polished enough to use in production apps immediately after the merge. Bug fixes/updates would make Seed less stable and we would have to release new Seed versions too often. - The new router isn't integrated into the older examples in the Seed repo, - I think reviewers would need to read at least a simpler description how the new router works to speed up the review process a lot. - How the new code affects compilation time, WASM file size, CI time, etc.? - It looks that the PR contains a new example, new router and Seed changes without clear boundaries. The directory/module structure or at least commits should divide changes to logic chunks. - etc. So I would suggest steps like these: 1. Extract the router and the new example into a standalone repo (we can move it under seed-rs Github org if you want). - Probably there are some Seed parts that we will need to modify to make the extraction possible. We would discuss them individually. 2. Write more examples (including some really simple ones). 3. Write docs - at least reasonable README.md + description for examples. 4. Improve API and fix bugs until we'll get confident enough that it's stable and users like it. 5. Decide if we should keep it the standalone repo or move to Seed or move partially (e.g. only improve Url or something like that). 6. Write official docs, docs comments, etc. specific for the router. 7. Merge / move to Seed / release a standalone seed-router crate. 8. Update seed-rs.org tutorials and Seed examples as necessary. 9. Release a new Seed version. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHFYCCJBMA5NFLUAHFTL7MTSM33JZANCNFSM4RE2GYKQ> .

[arn-the-long-beard]

Update from today :

@MartinKavik

It looks that the PR contains a new example, new router and Seed changes without clear boundaries. The directory/module
structure or at least commits should divide changes to logic chunks.

No, there is an error in the merge I done. There is no modification on seed source code.

Here is what I am going to do :

• Extract the router
• Extract the routes related macro
• Rebase and clean this PR

[arn-the-long-beard]

Hey @mankinskin

I extracted the code for Router & Routing there .

https://github.com/arn-the-long-beard/seed-routing

I could not keep the dependencies for enum_paths because I had so many changes to the code in the derive macro. But no worries 😄 . We can split this new repo as well , and then update enum_paths with it for the struct variant.

[arn-the-long-beard]

Hey @MartinKavik

I am always getting error from the ci with Clippy while everything is fine on my computer.

Is there something i am missing ?

[arn-the-long-beard]

OK

I had to run this cargo make verify_for_ci directly in the directory of the example to have the errors.

But then it crashed and I do not succeed to get the command.

I am stuck with

dependency `seed` does not specify a version
Note: The published dependency will use the version from crates.io,
the `git` specification will be removed from the dependency declaration.

[arn-the-long-beard]

@MartinKavik Hi 😄

Could you tell me what do you think about this example ?

[flosse]

I'd love to see a working example in the seed repo but I think until no seed-routing crate is published to crates.io we should just point to your repo and it's example.
So I'll close this PR now.
Feel free to reopen it :)