Skip to content
This repository has been archived by the owner on Jul 19, 2020. It is now read-only.

Minimal route derive #199

Closed
crackcomm opened this issue Dec 15, 2019 · 9 comments · Fixed by #204
Closed

Minimal route derive #199

crackcomm opened this issue Dec 15, 2019 · 9 comments · Fixed by #204
Labels
enhancement New feature or request low priority Other work will take precedence over this. Speed of work may be dependent on external interest.
Projects

Comments

@crackcomm
Copy link

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.

@crackcomm crackcomm added the enhancement New feature or request label Dec 15, 2019
@hgzimmerman
Copy link
Member

hgzimmerman commented Dec 16, 2019

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 #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.

@hgzimmerman hgzimmerman added the low priority Other work will take precedence over this. Speed of work may be dependent on external interest. label Dec 16, 2019
@crackcomm
Copy link
Author

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
Copy link
Member

hgzimmerman commented Dec 17, 2019

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.

@hgzimmerman hgzimmerman added this to In progress in 0.8.0 Dec 17, 2019
@crackcomm
Copy link
Author

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
Copy link
Member

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
Copy link
Member

hgzimmerman commented Dec 17, 2019

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
Copy link
Member

I intend to momentarily merge #204

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

0.8.0 automation moved this from In progress to Done Dec 17, 2019
@crackcomm
Copy link
Author

crackcomm commented Dec 18, 2019

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
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request low priority Other work will take precedence over this. Speed of work may be dependent on external interest.
Projects
No open projects
0.8.0
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants