Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Routing + link building + Url API #537

Open
MartinKavik opened this issue Sep 10, 2020 · 10 comments
Open

Routing + link building + Url API #537

MartinKavik opened this issue Sep 10, 2020 · 10 comments
Labels
API design Proposal for a new or updated Seed API. help wanted Extra attention is needed

Comments

@MartinKavik
Copy link
Member

Routing + link building + Url API are ones of the hardest things to design well and they are blockers on the way to the stable Seed core api.
Many related PRs and issues have been already created so I want to unite our effort in this "tracking" issue.

PRs:

Issues:

Motivation for the current routing

The previous API supported the app architecture, where you have one central router with Routes.
The Route was created from the Url and then the associated Page / page Model was created from this Route.

Problems:

  • The "wiring" among Url, Page and Route often introduces a lot of boilerplate (especially From implementations).
  • The central router doesn't work very well when you have many (nested) pages with dynamic parameters. You need to import many items from different pages to handle those Url -> Route -> Page transformations and it breaks encapsulation and introduces a lot of boilerplate.
  • You also need to implement the reverse process - build links from Routes. It means all pages have to have access to the central router - it breaks encapsulation even more and introduces another boilerplate when you want to have typed links (and you want to have typed links).

Also the previous routing / Url didn't take into account a custom base url path. So it was a Seed user's problem to implement it properly, unfortunately.

Feedback for the current routing / urls:

  • Url doesn't represent the entire url (it doesn't contain a domain, protocol, port, etc.).
  • It's surprising that Url is "stateful". (It's stateful because it works a bit like iterators.)
  • Link building and routing are independent. It may cause bugs when the developer accidentally implements them differently for the same route.
  • Routing is a bit exotic. Especially for guys coming from component-based frameworks because they aren't used to parse & match urls.
@MartinKavik MartinKavik added help wanted Extra attention is needed API design Proposal for a new or updated Seed API. labels Sep 10, 2020
@mankinskin
Copy link

Hi, I just released the enum_paths crate, which might make defining routes easier. Maybe we can use it? Basically it derives methods for nested enums to convert them from and to / separated strings.

@mankinskin
Copy link

This is a router that I implemented using subscriptions to the current subs::UrlChanged messages and enum_paths:
https://github.com/mankinskin/binance-bot/blob/master/src/client/router/mod.rs

I am not sure if there is anything missing, but all of these work:

  • clicking links
  • changing the url in the browser search bar
  • opening site with specific url
  • navigating history

Also there is no generated mapping between enum Route and enum Page, which causes some boilerplate. But it is not as tedious as converting strings to an enum, and I am not sure how to implement such a mapping correctly, so that it works for all cases and isn't limiting.

There also is the boilerplate in the update function of a parent component, because those always need to forward proxy messages to their children. But that is another issue.

@mankinskin
Copy link

Another point might be refactoring the Url type, to make it more compatible with url paths in String shape and make it contain the full browser url including protocol, host, and all kinds of queries, paths, etc.

@mankinskin
Copy link

For anyone interested, my components crate just gained a generic Router component. It listens to UrlChanged and will switch the page by parsing the url path to a Route and initializing the page with it. It is pretty simple, but should do the job. It does require the components crate though.

@arn-the-long-beard
Copy link
Member

Hello guys,

Well seed_routing is under active development and already has several features.
Any help is welcomed 😉

Here are 2 issues I have just created : https://github.com/arn-the-long-beard/seed-routing/issues.

There are several steps we need to complete before publishing it as a crate or integrating into Seed

  • Good naming for Apis.
  • Enough integration/unit test.
  • Enough features.
  • Solid and maintainable implementation.
  • Good documentation

Let me know if you have some inputs and/or have questions 😉

@arn-the-long-beard
Copy link
Member

Hello guys,

We are still working on the seed_routing and making improvements and polishing it.

Hey @mankinskin , would you like to take a look at it ? 😄

@mankinskin
Copy link

Hi @arn-the-long-beard !

Wow, looks like you've come a long way since I last looked into that. Personally, I would keep polishing this and provide it as a decent utility crate, and maybe look to integrate it into seed natively in the future.

The seed_routing crate adds a lot of features to remove boilerplate, and usually this comes at the cost of taking away some flexibility as you are fixing some rails for fast progress. I think there are basic tools in seed already for handling routing and building your own routing component, so this is not something with high priority for me right now. But this can certainly make building apps with seed faster, when it is well integrated.

One thing I am still confused about is why you built a feature for managing the browser history, when the browser has a built in system for that? Does it do anything differently? Do you want to bypass that and do processing on the browser history on your page? Would be glad if you could explain this to me :)

@arn-the-long-beard
Copy link
Member

arn-the-long-beard commented Feb 17, 2021

Thank you for your message 👍

One thing I am still confused about is why you built a feature for managing the browser history, when the browser has a built in
system for that? Does it do anything differently? Do you want to bypass that and do processing on the browser history on your
page? Would be glad if you could explain this to me :)

It was written as requirement in one of the issue #383 maybe I misunderstand it . But yeah put it this way, it looks very useless that I built it. I forgot we could have access to it this way https://developer.mozilla.org/en-US/docs/Web/API/Window/history actually.

On the other hand, I have never used the one directly from the browser in Angular and I have just discovered from your post that it is available in web_sys. Used the router from Angular to do this . I got a bias it seems.

I do not know what to think about it right now. Maybe the whole thing is not needed.

@mankinskin
Copy link

Ah okay, that clears it up, I kept wondering if I was missing something. I have never used Angular, only React a very little bit, so this seemed strange to me. But an abstraction of the browser history is actually really useful I guess, so you don't have to use low-level web_sys all the time. But I think it should really just be a shell to the browser history and not have any state itself, if it isn't for caching or things like that.

@arn-the-long-beard
Copy link
Member

Yes it makes sens. Probably what I was using in angular was calling the browser Api actually. I am almost sure now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design Proposal for a new or updated Seed API. help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants