Suggestions for Url-Api

[samuelpilz]

I did a review of the url-api. My motivation is that I did not understand how this module is intended to be used and saw an opportunity to improving discoverability and perceived consistency of the api.

I believe I understand the intention behind the provided functionality. I understand its usecases for routing and I do not want to remove any features from the module.

Name the new parts of the url

The Url struct is not the typical url. It represents more than just the url (it has extra features for routing). All parts of a typical url are already named corretly (path, hash, search). The "Base path" is a concept introduced by the routing-functionaliyt and is implicitly named.

I suggest embracing concept of the base-path. In current terms, it is the parts of the path that are "behind" of the url. The functions to_base_url and skip_base_path hint at that fact.
If there is a "base-path", there should also be a name for the rest. I see two possibilities:

• path = base-path + head-path. The name head_path is the best name I could came up with, it is to be bikeshed.
• absolute-path = base-path + path. This would be a breaking change and is quite disruptive.

The following describe suggested changes to functions:

to_base_url

This method clones the url and truncates the path. Suggestions:

• The method should take &mut self to avoid cloning inside the library - users can still clone if they like to.
• The method does not truncate the hash, which it should. Conceptually, the hash comes "after" the path.

next_path_part

Instead of "Advances the internal path iterator", the documentation of this method desccribe somthing along the lines of: "the next part of the url is moved to the base-url section of the path".

return string slices

• implement base_path(&self) -> &[String].
• remaining_path_parts should be head_path(&self) -> &[String] (no mut, return slice instead of vec)

documentation

consistent subtitle

URL used for routing can serve as a subtitle for seed's concept of url and could to be communicated more. This highlights that this is not the typical url-struct (just data) and serves the specific purpose. It should also documented that the url-path is split into

• module-level description should start with that
• struct-description should start with that (it already does)
• guide-documentation

const DUMMY_BASE_URL

This is a const to "example.com". This seems a bit odd, currently the url-struct represents relative urls (which is nice). Having this const in the framework does not make much sense.

struct_urls macro seems off

It generates a newtype around a base-url. I understand that the custom methods (like home()) can be useful, but I doubt that this is the best abstraction.
I suggest removing the struct_urls! macro and having the users require to store the base-path inside the model. The functions like home() can still be implemented somewere.

reorder URL-methods

I had a difficult time understanding the nature of the url-module because the main mehtods for the entry-level user are quite hidden. Is there any possibility to reorder the methods?

• getter (path, search, ...)
• action methods go_*
• setter
• iter

Alternatively, one could extend the struct-level documentation to include examples about the most commonly used functions in simple apps (path, next, ...).

[MartinKavik]

This would be a breaking change and is quite disruptive.

The entire new Url logic is only in the master - feel free to change it as you want. It would be nice to have a good and documented Url API in Seed 0.7.0.

---

My motivation for that refactor was simple - I had one big router and a struct/function with links in one of my app and it became very error-prone, so I tried to find some better patterns. Current Url API is the result of this effort.

So.. current Url/routing API is better than the old one but I agree with you that it can be better. I don't see any problems in your proposals - feel free to create a draft PR with your suggestions so we can see and discuss the whole picture and patterns - especially in examples.

[samuelpilz]

I will draft a PR for this later this week.

[AlterionX]

If @power-fungus has no objection (or doesn't respond in a few days), could I create the PR instead?

I have a few further suggestions They mostly amount to integration with existing Iterator infrastructure and exposing the object further.

The first is to convert UrlSearch::new into an impl of FromIterator and repurpose UrlSearch::new for constructing an empty UrlSearch (I guess like what the Default trait does?) for consistency with std::Vec and assorted containers, since UrlSearch seems like a relatively thin wrapper around BTreeMap with a few modification. I would also like to change the impl Into<UrlSearch> into UrlSearch to have certainty about what arguments can be passed into set_search, doubly so because I haven't found anything that explicitly impls From<T> for UrlSearch or Into<UrlSearch> for T yet outside of UrlSearchParams from web_sys.

The second is to remove all the iteration methods on Url and, instead, allow the existing std iterators to be use (such as using url.path().iter().next() and keeping the iterator around instead of url.next_path_part()). This is to separate the concern for storing the Url and iterating over it. Not sure how I would go about implementing it, but that's related to the third suggestion. The subsequent iterators can be passed around as impl Iterator<Item = str> or type aliased.

The third and kind of questionable is to expose the search, path, and hash_path fields directly, with a getter for the hash instead of caching the hash.

The second and third are afterthoughts, but the first, I think should be implemented, especially as it's a relatively minor change for the API users that adds consistency with the std. This change is mostly due to my struggles with figuring out how to construct an empty UrlSearch and filing that up directly instead of constructing a Vec or other intermediary structs since my query was dynamic.

[MartinKavik]

@AlterionX Try it and please update also examples to match your changes:

• url
• pages
• pages_hash_routing
• pages_keep_state
• auth
• todomvc
• unsaved_changes

Clean Url API calls and patterns in these apps/examples have higher priority than idiomatic Rust (e.g. usage of native Rust Iterators) from my point of view.

[AlterionX]

Oh boy, that's a bunch. Also, got it.

[samuelpilz]

Please go ahead with a PR. Unfortunately, the seed-project I was working on has been suspended and brought my interaction with seed to a halt.

I have a few comments which may or may not be considered during implementation.

The current implementation already allows url.path().iter(), which is impl Iterator<Item=&String>. The function next_path_part is indeed a bit weird, but thinking more about the intended usecases, something like it does make a lot of sense. I would advise against removing it and saying "use path-iterators instead".

The notion of the base-path is already present in the current api and my proposal is to embrace the concept for mutable url structs. Maybe the next_path_part function can be renamed to describe better what happens: move a single path part to the base-path section. (maybe advance_base_path, probably there is something better)

When I worked on this issue, I aimed for a more complete api for routing. In my project, I had to deploy my app at foo.bar/myapp/. In react, I would set the public-path environment variable at build time, which would get all routes correct in the static files and js-routing. However, this is not really easy in seed. I do not want the /myapp/ part of the path during in-app-routing. Base-paths could be used to solve this problem. Whataver the solution is, this problem should be thought about.

During my work on the url-code, the DUMMY_BASE_URL const is quite weird. It turned out to be difficult to remove: The urls from web_sys need to be absolute and conversion to a web_sys-url is performed by applying the DUMMY_BASE_URL The Display impl relied on such a conversion. Also somewhere in the AppConfig, it is required. Optimally, this can be refactored. Also there is the function clone_base_path, which I do not understand at all. Is there some better way to support the usecases it currently fills?

The features about public-path and web_sys-url independence are not too trivial to implement and I did not find a good solution yet.

I did not include hash or search in my proposal, I believe the discussion about these fields to be independent - any suggestion is fine to me.

[MartinKavik]

Also there is the function clone_base_path, which I do not understand at all.

The path from your <base href="/base/path/"> is stored in the App during App::start. You can get it by calling orders.clone_base_path() if the value is important for you. (It's stored in App to prevent calling slow JS + DOM methods.)

the seed-project I was working on has been suspended

It's shame, I hope it will be resumed 😢 I like you ideas and also it would be nice to see more high-quality Seed apps.

[samuelpilz]

The path from your <base href="/base/path/">

What is that? It looks interesting

[MartinKavik]

@power-fungus See #369 (comment) - there is an explanation and examples that exercise it.

[MartinKavik]

Just note: There is a related issue - #383