Url API changes

[AlterionX]

Mostly cosmetic changes, reorganization, and edits to the documentation.

The biggest change here is probably with the UrlSearch struct's FromIterator impl as well as how UrlSearch has been extracted into seed::browser::url::search (but reexported).

The documentation part was left out, mostly because I'm not entirely sure what would qualify as "most common" use cases.

Should handle most of #433, but all changes are up for discussion.

Also related to #383.

[AlterionX]

I need to find a linux machine somewhere to run cargo make test at some point, or figure out how to make WSL 2 run the tests -- as it is, the tests are failing due to lack of packages, incompatibility issues, etc.

[MartinKavik]

I need to find a linux machine somewhere to run cargo make test

Why? What's your problem and OS?

[AlterionX]

Windows, WSL 2
I'm probably just missing a few packages, but it's late and I wanted to put it off for tomorrow.

[MartinKavik]

I don't know if it's better or worse: two calls + hidden type VS explicit type UrlSearch that needs to be imported.
Do you think we can write only .set_search(vec![("x", vec!["1"])]) or set_search(UrlSearch::new..) and don't complicate set_search parameter too much?

[AlterionX]

Apparently some people agree about clippy's filter_map lint. rust-lang/rust-clippy#3188

Would it be okay if I disabled it?

[MartinKavik]

Could you use more expressive names? I.e. k -> key and vs => values? (It applies also for other cases)

[MartinKavik]

Windows, WSL 2

I use Windows without WSL, with git-bash instead of cmd/ps and it works without problems.

Apparently some people agree about clippy's filter_map lint.

I've already disabled it somewhere, I won't cry if you do that.

[MartinKavik]

• url.to_base_url() -> url.clone().truncate_relative_path()

  • relative_path implies that there is also an absolute_path but Url uses only relative paths from the user point of view. Also I'm afraid it will be even more confusing once we add support for real relative and absolute paths.
  • Alternative names: url.clone().trim_path(), .trim_path_end(), .shrink_path_to_base(), ..
  • Or leave it as is - is domain-specific naming and implicit cloning bad? And prefix to_ also implies that to_base_url returns Url; I would expect that truncate_relative_path returns ().

• url.next_path_part() -> url.pop_relative_path_part()

  • pop usually removes the last element from collections - we don't remove anything here and it should be named pop_front to respect conventions => I would leave .next_path_part as is.

• self.base_url().add_path_part(LOGIN) -> self.base_url().push_path_part(LOGIN)

  • I agree, push makes more sense than add.

• url.remaining_path_parts().as_slice() -> url.consume_relative_path().as_slice()

  • What do you think about collect instead of consume? This method only borrows, doesn't consume path parts. So it would change to .collect_path or .collect_remaining_path to be super expressive.

(cc @power-fungus)

[MartinKavik]

Just idea for future explorations - Cursors - perhaps it would help with implementation.

[AlterionX]

Thanks for the comments. Next, I've been thinking about what you said, and wanted to write something comprehensive and well thought out. I'm sorry for the wall of text.

I think we need to talk a bit about what the two parts of the path should be named -- ideally something that speaks more to the intended purpose of Url than remaining_path. Also, it's not immediately obvious what remaining means--the word remaining doesn't seem to pertain to the Url. I agree that relative_path is confusing, but I think it's more declarative than remaining_path and I think it's better than head_path as suggested, or tail_path since there is no head_path. I'm not I don't want to rename base_path to head_path either, since I think base_path speaks extremely clearly to what it's meant for. The names parent_path and child_path also popped up, but I don't like those either. As such, I've been unable to come up with a good name for that part of the path. Taking this into account, I have added a small snippet to the docs of functions pertaining to the base and remaining paths to clarify their meaning.

I also think that Url should be named something different like UrlPath/WebPath/SitePath (not AppPath since it's not directly related to the application), since it is both more than and less than web_sys::Url. Also, it'll avoid confusing people if someone sees Url and assumes it's an actual Url. The reason I avoid untyped languages is due to their uncertainty about what data ends up where. The name Url in this case makes me uncertain about it's difference with web_sys::Url. I think that doing this in the code would make it more obvious to everyone involved about how they're different.

Sidenote: The second part's suggested change may mitigate the confusion from the first part, if we go down that route. However, I still think that there should be a better name than what we have come up with so far.

Third, regarding the consume_* methods or Url, I was also trying to come up with a better name. The key part here that I was attempting to highlight is that these two methods "eat up" the rest of the relative path, shunting their path parts into the base path. I think that collect_remaining_path doesn't exactly imply that this happens. I expect collect_* to create a Vec that's populated with whatever * is, but not change the Url. (My brain thinks a little differently for Iterators, hence the apparent inconsistency there. Another reason I wanted to avoid collect, because it's almost but not quite an iterator.) To that effect I've written a new method -- relative_path -- that would do what I expect collect does, short of allocating a Vec.

Fourth, regarding to_base_path/truncate -- I wanted to make it return nothing and just truncate the path, but was worried it might be a big change. That said, I will change it to return () instead soon, if that's okay. I had issue with the to_base_path method since it didn't consume the original Url. Another part was that I wanted people to avoid thinking that truncate acted on the original Url, so I tried to make it obvious that truncate needed ownership to act, but that's honestly not that important.

Fifth, regarding the change to set_search, I think that making the conversion here noisy is better, since I would personally prefer a macro for dictionary construction than implicitly converting a Vec into a BTreeMap, doubly so since it's not immediately clear that a Vec should map to BTreeMap. That said, we can use the UrlSearch::from_iter method, but that would require importing the FromIterator trait (see FromIterator's documentation for more details regarding from_iter and how it's not typically called by itself). It wouldn't be hard to write another method to avoid importing FromIterator, but I still think that such a function should be avoided. In conclusion, I think the real solution is to write a macro for this particular case as from the maplit crate, like so:

url_search! {
    "query_param0" => ["data", "thing"],
    "query_param1" => [],
    "query_param2" => "other thing",
}

Lastly, regarding pop_*, I was thinking of a longer method name, but was worried it would take too long. Thinking on it a bit more, I suppose I don't like pop_* after all, but I also don't like next_path_part since I don't think that Url should act as if it has an internal iterator--again, we would be mixing concepts that don't usually go together. Now if it just didn't fit together, I wouldn't have much of an objection, but this paired with the fact that the struct is more than just an iterator makes me feel like it's rather confusing what, exactly, is going on. Having said that, I was thinking of shift_next_path_part_to_base since next_path_part sounds more like a getter outside the context of an iterator, but as mentioned, it felt like it was too long. Perhaps we can break this up into two methods or rename it so that the function's purpose as well as what it does is a little clearer.

Another sidenote: I left a bunch of TODOs, could you take a look and see which ones seem reasonable? They're there since this was supposed to be closer to a discussion.

[MartinKavik]

I've just read it quickly so keep it in mind while reading notes below:

Also, it'll avoid confusing people if someone sees Url and assumes it's an actual Url.

• There was/is plan to make it full-fledged Url - it should replace web_sys::Url. Just like we have wrappers for fetch and websockets.

I expect collect_* to create a Vec that's populated with whatever * is, but not change the Url.

• Well, I think of Url as a special Iterator - basically a container for multiple iterators. So I expect it changes Url a bit - just like standard Iterator.

That said, I will change it to return () instead soon, if that's okay.

• I think it would decrease developer experience a bit because you can't call it and store the new Url into your Model on one line (?).

I had issue with the to_base_path method since it didn't consume the original Url.

• Yes, that's why the prefix is to_ and not into_ to respect naming conventions and make it explicit.

Fifth, regarding the change to set_search

• I would design it simple as possible - UrlSearch::new + url_search! (as you suggested) - basically like Vec::new + vec![] or other collections and that maplit crate.

next_path_part since I don't think that Url should act as if it has an internal iterator--again, we would be mixing concepts that don't usually go together

• Well, how I said, it's basically Iterator for me and it should be easy to grasp the concept because Rust developers already know Iterators. I think we won't agree on this - perhaps we'll need more opinions.

[AlterionX]

Okay, so let's backtrack a little here, since I think what I'm trying to convey is getting a little lost in the details.

Can we agree that a struct should have one "role", so to speak? The single responsibility principle from OOP, as it were. Url as it is now has two separate roles: it attempts to be an iterator as well as represent the url. From it's name, I only notice the second, not the first. It requires some deep delving in the docs to conclude the first purpose of Url. It says "Url used for routing," but that doesn't explain the internal state or what it's supposed to represent short of the fact that routing is somehow involved and it's a url. Again, the second, and maybe a hint of the first purpose.

This leads to what I believe to be the most important part of my response earlier: I want to rename Url as well as name the relevant components. Here's the paragraph:

I also think that Url should be named something different like UrlPath/WebPath/SitePath (not AppPath since it's not directly related to the application), since it is both more than and less than web_sys::Url. Also, it'll avoid confusing people if someone sees Url and assumes it's an actual Url. The reason I avoid untyped languages is due to their uncertainty about what data ends up where. The name Url in this case makes me uncertain about it's difference with web_sys::Url. I think that doing this in the code would make it more obvious to everyone involved about how they're different.

Before quibbling about the methods and whatnot (not to say that these details are trivial, just secondary to the primary point), I wanted to establish a clearer picture of what the purpose of the Url struct is and is not.

I think we can arrive at an agreement if we can decide on a new name for Url that better captures what it is or split Url's functionality into several components. From your description, however, I'm a little worried about Url adopting even more varied methods and it's purpose (read responsibility) getting more muddled than it is now.

[MartinKavik]

Can we agree that a struct should have one "role", so to speak? The single responsibility principle from OOP, as it were. Url as it is now has two separate roles: it attempts to be an iterator as well as represent the url.

I'm aware that Url:

1. Contains "custom" Iterators.
2. Doesn't represent Url correctly (it doesn't support absolute path now).
3. Is mixed with routing (e.g. you can push route into the browser history through Url methods).

And that everything above is bad on a paper.

So yes I also

wanted to establish a clearer picture of what the purpose of the Url struct is and is not.

But all my attempts to write idiomatic Rust failed when I tried to integrate them into bigger examples and real-world apps.

It says "Url used for routing," but that doesn't explain the internal state or what it's supposed to represent short of the fact that routing is somehow involved and it's a url.

I think I just copy/pasted old comment because I didn't have time to write something better and I didn't know all problems and patterns to write it properly.

I think we can arrive at an agreement if we can decide on a new name for Url that better captures what it is or split Url's functionality into several components. From your description, however, I'm a little worried about Url adopting even more varied methods and it's purpose (read responsibility) getting more muddled than it is now.

• The goal was to use Url as a wrapper for web_sys::Url. If the goal is still valid, then renaming doesn't make sense.
• DX and usage in examples and especially in bigger real-world apps have priority. I don't care about theory and a bit steeper learning curve if it helps people in the long run. And there were some positive feedbacks on Seed chat. So - I don't want to change Url until it's clear that it helps with writing apps and we have proves in examples.

So.. I agree with you that there are some Rust/OOP/SoC issues, but there are considered trade-offs. So please show us how your changes improve user life while he is writing more complex apps if you want to change API.

[samuelpilz]

I am happy to see that @AlterionX 's suggestions and changes go in a direction I quite like.

Some thoughts:

• Having relative-urls (also relative to the app-base-path) is a surprisingly useful feature for routung. The plan to also support absolute urls (for CORS-requests) should not get in the way of the routing api (by making absolute paths optional).
• Regarding iterators:
  • conceptually, web_sys::Url is a stateless struct, adding state is quite a surprise
  • using iterator-getters (url.path().into_iter()) instead of mutable state should work in all cases I can think of and is certainly the more expected api for newcomers.
  • If the iterator-nature of Url should be kept, the name should represent
• is the splitting of the hash into parts really beneficial? Is the character / the obvious segment seperator? Does any other framework set / as hash-separator? If not, I suggest returning to hash() -> &str
• @AlterionX in your comment, you discuss naming preferences. Did you get to any conclusions? Personally I would prefer one of
  • <base_path>/<head_path> (tail_path would also work)
  • <base_path>/<path> (this one might be a bit unexpected, since path() automatically skips the base.

[MartinKavik]

using iterator-getters (url.path().into_iter()) instead of mutable state should work in all cases

It doesn't work when you want to handle each path part and other url components in the associated page module. See some examples - e.g. pages. Better alternative welcome.
And motivation for handling in page modules - one central router becomes messy when you have many pages.

is the splitting of the hash into parts really beneficial? Is the character / the obvious segment seperator? Does any other framework set / as hash-separator? If not, I suggest returning to hash() -> &str

There are two methods - hash() returns Option<&String> (it basically corresponds with your hash()->&str) and the second one - hash_path() - returns &[String] (the same output like path()) and it's for users who need hash routing because of conflicts with backend endpoints.

[samuelpilz]

looking at the pages example, one could change the init functions to be one of:

• init(mut url_path: impl Iterator<Item=&String>), instead of url.next_path_part() one could call url_path.next()
• init(url_path: &[&String]), instead of url.next_path_part() one could call url_path.first() and instead of passing the same url struct to the child init-function, one could pass &url_path[1..] instead.

If any other additional information about the rest of the url would be required, one would need to add additional parameters - with the additional benefit for documentation that these parameters need to be named.

[MartinKavik]

@power-fungus Yeah, it's one reasonable alternative.
I didn't want to go this way because:

• I can't predict what users will need - it would be sad if it would force them to pass multiple iterators / arrays / strings. Also it would be quite error-prone because you basically pass only Strings/&strs (lost expressive/domain-specific names).
• I don't know if it would improve link building or make it worse.
• How would it work with base paths?
• Types like mut impl Iterator<Item=&String> would reduce readability.

However I can be wrong and this API would be better in practice.

[samuelpilz]

I agree about not predicting the usecases of users. It has proven to be a useful heuristic to favor designs that do less but are more composable. seed's view system is a good example of this.

I made a mistake: mut is part of the pattern, not the type. It should go init(mut url_path: impl Iterator<Item=...>). Thinking about mutability is a first-class concern in all Rust programming and is common enough to not hinder readability.
Personally, I would prefer the slice-passing-option, but there are also other options (in the spirit of composability)

Link building is a different consideration (in my view). Probably, it is fine to use the current push_path_part on the base-url.

A hybrid approach exists: pop_relative_path_part (as in this PR) that changes the part of the path that is considered "base". I would not consider this advance an iterator or cursor, but rather as a mutation of the url-struct (similar to a write to a collection).

[flosse]

This PR seems to be outdated and there was no chance since 2020 so I'll close it now. Feel free to reopen it!