yewstack / yew_router

A routing library for the Yew frontend framework
Apache License 2.0
90 stars 23 forks source link

Minimal route derive #199

Closed crackcomm closed 4 years ago

crackcomm commented 4 years ago

Hey, I love this library but unfortunately, I had problems with stdweb compiling and it was related to emscripten. I want to use this library on the server side so I decided to remove everything including route state and was left with clean and simple AppRoute::from_path(req.uri().path()) which now I can use with match and make my own type-safe router for API. I am still removing what I don't use in my fork. I'd love to see this library being equivalent to serde-derive for routers.

Thanks.

hgzimmerman commented 4 years ago

Yeah, this library will not work in non-stdweb compatible environments.

While you are welcome to maintain a fork, I don't currently want to take on the additional responsibility of making sure this works outside browsers as this library was only developed for being used within the context of a yew application (or possibly another wasm frontend framework).

To remove Stdweb currently means that the Route struct wouldn't be representable any more (or would otherwise break compatibility with its use in the frontend). That will hopefully change after https://github.com/yewstack/yew_router/issues/185 is implemented and stdweb trait bounds (JsSerialize) can be removed.

I would like to point you towards Rocket and Warp, which while not using a enum to describe their routes, do use macros to extract data from request paths.

I don't want to say no here, but implementing server-side support isn't a goal of this project, and other, dedicated backend frameworks have solved this problem better for use in a server-side context. I think it might be supported eventually (by making the minimal feature even more minimal), but it isn't on any near-term roadmap.

crackcomm commented 4 years ago

I completely understand. I forked it myself and it's just fine. My recommendation is to consider this on the next iteration or refactorization which may or may never happen. Until then I'm very happy using my own fork of yew_router. Thanks!

hgzimmerman commented 4 years ago

I've gone ahead and made the service module a feature. This means using the library without any default features should allow you to use this library in non-web environments.

Can you add the following as a dependency for your project and let me know if it resolves your problem?

yew-router = {git = "https://github.com/yewstack/yew_router", branch = "feature-gate_service", default-features=false }

I still don't want to constrain myself to guaranteeing that this library will not require yew or stdweb when using it without any features, but I will do my best to avoid that scenario.

crackcomm commented 4 years ago

Could you include this commit (without removing From<Option>)? I think it's equivalent and I need it to deserialize path elements and I think it's a good feature to include with less code.

hgzimmerman commented 4 years ago

If I recall correctly, the reason I broke the implementation out into a macro was to allow Switch to be implemented on Option, because coherence rules said that if std added a FromStr for Option, then there could be a set of overlapping implementations.

I absolutely would like to get something like that working, but I think it would require sacrificing that support for Option. I'll give it a shot and get back to you.

hgzimmerman commented 4 years ago

Try out the latest commit on that branch. I had to remove the Switch implementation forOption, replacing it with a Permissive wrapper around Option instead and Route::<()>::from("string") no longer works, and you must use Route::new_no_state("string") instead.


Versus what your commit contained: The API will always use a Route because the frontend must deal with the possibility of storing state within the browser's history. But the Option<STATE> (Option<T>) should be going away in the near future, being replaced with just STATE, but the route String is probably going to remain a String, although maybe a Cow<str> might be used instead.

hgzimmerman commented 4 years ago

I intend to momentarily merge https://github.com/yewstack/yew_router/pull/204

Try out the latest master instead get access to the changes embedded in there.

crackcomm commented 4 years ago

It does work, awesome. I only miss one thing, in my fork I could do:

match App::from_path(req.uri().path()) {}

Now I am using:

let (route, _) = App::from_route_part::<()>(req.uri().path().into(), None);
match route {}

Maybe even this would be possible:

match req.uri().path().into() {}

Using impl From<&str> for Route<()> and impl<S, T: Into<Route<S>> From<T> for SwitchImpl.

I really appreciate your work and now I certainly will not have to maintain my own fork and .into() is just something you could think about and if I can help let me know.

Thanks again!

hgzimmerman commented 4 years ago

There should be a Route::new_no_state or Route::new_default_state to create a Route<()>.

Using this you could make it a little more terse with something like:

let route = App::switch(Route::new_default_state(req.uri().path().into()));

Additionally, I think its kinda dumb that you still have to reallocate the &str into a String in order to parse it, but switching to another design would break core functionality of this lib. If your framework takes off, you may want to re-investigate forking to avoid that cost, but for now I don't think its worth it

If you submit a PR to add a blanket impl that doesn't cause any more API breakages, I'd happily merge it, but for now I think I'm done working on this problem-space.