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

[sabine]

Just two short points (overall, I'm happy with the Url API and routing facilities of Seed):

1. I would rename UrlSearch to UrlQuery because query is overall the more commonly-used term for that part of the URI. Users new to web development in general will get better search results when they search for a "url query" instead of a "url search".
2. I was surprised a little by the behavior of https://docs.rs/seed/0.7.0/seed/browser/url/struct.Url.html#method.go_and_push. Considering it is named "go" and "push", I expected that it would do both: a) push the URL via the browser history API, and b) trigger the UrlChanged subscription. Since it only does a), I think push is a better name.

Note on 2: it is good that the function doesn't also trigger the subscription, because that leaves it in my hands to decide what to do inside my app. As a user of Seed, I can
a) send the same message I use for the UrlChanged subscription, when I do a programmatic redirect, and
b) use another message (or no message at all) when I updated the URL to reflect UI changes.

Another Note on 2: The documentation should give an example of a programmatic redirect, e.g.

fn navigate_to (orders: &mut impl Orders<Msg>, route: Route) {
    let url = seed::browser::url::Url::from(&route);
    seed::browser::url::Url::push(&url);
    orders.send_msg(Msg::UrlChanged(subs::UrlChanged(url)));
}

[MartinKavik]

@sabine

---

Ad 1. - It's named UrlSearch because it was inspired by https://reasonml.github.io/reason-react/docs/en/router.html and because it corresponds with Web API https://developer.mozilla.org/en-US/docs/Web/API/URL/search. However your arguments for UrlQuery are good and query is also used in https://docs.rs/url/2.1.1/url/struct.Url.html#method.query. I would like to hear more opinions.

---

Ad 2. - My reasoning - all functions that modifies the url directly in the browser window have prefix go (go_and_push, go_and_replace, go_back, etc.). They are basically slim wrappers for native History and Location api. So some of those functions trigger push_state event (Seed listens for it and then fires UrlChanged) and some of them don't - and I didn't want to change this native browser behavior until it's clear it should be improved by Seed.
A related example is https://github.com/seed-rs/seed/blob/master/examples/url/src/lib.rs (you can run it in the Seed repo root by cargo make start url).

Programmatic redirect should be done by:

seed/examples/url/src/lib.rs

Line 61 in a0e01ae

orders.notify(subs::UrlRequested::new(url));

I plan to add it into seed-rs.org guides. And maybe it's somewhere in the docs already.

You can also subscribe to subs::UrlRequested and interrupt it / modifies it / prevent UrlChanged firing. That way you can handle manually both programmatic redirects and <a> link clicks.

UrlChanged and UrlRequested behavior is inspired a bit by https://package.elm-lang.org/packages/elm/browser/latest/Browser#application.

So I don't have a strong opinion about it - I haven't got problems with UrlChanged firing personally but I can imagine it can be surprising.

I think push is a better name.

• I'm afraid that some users would understand it as an alternative to something like add_path_part.

Possible next steps:

1. Leave the API as is.
2. Add a new function Url::go(&self) that fires subs::UrlRequested automatically.
3. Fires UrlChanged also while calling functions like go_and_push.
4. Rename go_x functions.
5. Extract go_x and functions like reload into the standalone module and rename them.
6. ?

[sabine]

all functions that modifies the url directly in the browser window have prefix go (go_and_push, go_and_replace, go_back, etc.). They are basically slim wrappers for native History and Location api. So some of those functions trigger push_state event (Seed listens for it and then fires UrlChanged) and some of them don't - and I didn't want to change this native browser behavior until it's clear it should be improved by Seed.

There's nothing wrong with staying close to the browser specs. That makes it easier for experienced folks to move to Seed because things work as expected.

Only thing that tripped me was the term "go" which, due to lack of more details in docs, I interpreted as firing an UrlChanged sub. (But, as I said, it's good it doesn't do that. This leaves control in my hands, whether I want that to happen or not.)

I'm afraid that some users would understand it as an alternative to something like add_path_part.

Ok, that's definitely possible. You could name it history_push, history_replace, history_back? This way, someone searching for "history push" on the internet will get decent search results and someone who knows the history API will immediately know what it is.

3. Fires UrlChanged also while calling functions like go_and_push.

No, please don't do that, you would be taking away from the flexibility we have now. There are situations where it makes sense to change the URL (in-place or by adding a new history entry) in the browser without going through a full "page changed" scenario inside the app.

Good next step IMO:

1. clean up names with the purpose of making it easier for new users to understand from google search or previous knowledge what the function is supposed to do (search -> query, go_and_push -> history_push, go_and_replace -> history_replace).

So generally, leave API as is, there's nothing wrong with being a thin wrapper, and I don't think adding anything on top of what you have will make it better.

Only thing that would be convenient to have is automated serialization/deserialization of routes, but that's a luxury feature that almost no one has.

Your examples are great, and they do contain the critical pieces. Still, it can be hard for a newcomer to take all that good info in, and find the right example at the right time. With routing, in particular, things seem to have changed over the last versions. One thing that could help is to compile a FAQ that works like this website: http://microjs.com/ i.e. there is a list of the things that people commonly ask how to do them, and then, as an answer, the site shows one small paragraph that explains, and a link to the corresponding example. So, there would be "I need to..." and a choice box. One choice: "redirect to another URL". Answer: "The Seed way to work with the browser URL is to use the URL subscription API (link). All you have to do to redirect to another page is to notify the UrlRequested subscription like this: orders.notify(subs::UrlRequested::new(url))".

[sabine]

You can also subscribe to subs::UrlRequested and interrupt it / modifies it / prevent UrlChanged firing. That way you can handle manually both programmatic redirects and link clicks.

Btw, another way to manually handle link clicks is currently to use preventDefault and preventPropagation on the link in question. Intercepting subs::UrlRequested seems more convenient, though.

[MartinKavik]

clean up names with the purpose of making it easier for new users to understand from google search or previous knowledge what the function is supposed to do (search -> query, go_and_push -> history_push, go_and_replace -> history_replace).

Ok, I agree. I'll come back later to this issue (before Seed 0.8.0 release) and we can discuss better names (here or on the chat).

One thing that could help is to compile a FAQ
seed-rs/seed-rs.org#71