csells / go_router

The purpose of the go_router for Flutter is to use declarative routes to reduce complexity, regardless of the platform you're targeting (mobile, web, desktop), handling deep linking from Android, iOS and the web while still allowing an easy-to-use developer experience.
https://gorouter.dev
442 stars 96 forks source link

Query Params vs URL Params #50

Closed talamaska closed 2 years ago

talamaska commented 2 years ago

I find it a bit confusing that both are mapped to state.params. Is there a reason for that? Would you consider actually adding state.queryParams for query parameters?

csells commented 2 years ago

It's just a bag of name-value pairs that shouldn't overlap. I could think of no reason to keep both in separate spots and I prefer a smaller API surface when I can get one. Do you have a good use case for two separate collections?

talamaska commented 2 years ago

Well, in Angular router we have both of them handled separately. In reality you could have same name params, query params obviously are those after the question mark in the end of the url, path param is part of the url could be in the middle. I know you know this. Path param could lead to a separate page/widget, while query params cannot and can be used only to provide data to any route. I mean if your making a router that should work on web, you should have them distinguished. I can't imagine any of the web frameworks dropping this separation.

csells commented 2 years ago

Correct. That's the behavior go_router provides. My question is, is there a reason to have inline and query params in separate scopes? Do the names need to overlap for any reason?

talamaska commented 2 years ago

They don't need, but they could. I mean you could ask the Angular team why they have them separated in different collections. For me it gives more freedom, while if they are in a same collection it is an artificial / unnatural limitation of what web normally provides.

csells commented 2 years ago

It's only a limitation if there's a reason for separate scopes. Perhaps someday I'll need to move to inlineParams and queryParams. In the meantime, combining them keeps the API smaller and simpler.

talamaska commented 2 years ago

But you already have a logic that handles them separately, you are merging them artificially into one collection then you are making additional logic to split them.

csells commented 2 years ago

I do have logic to join them for the pageBuilder. What logic splits them?

talamaska commented 2 years ago

https://github.com/csells/go_router/blob/master/lib/src/go_router_impl.dart#L809

csells commented 2 years ago

I do that to keep the API simple for navigating to named routes -- anything that isn't a positional/indexed parameter is added to the end as a query parameter.

I'm not worried about keeping the implementation simply; I worry about keeping the API and the end-to-end developer experience easy.

talamaska commented 2 years ago

I'm not sure what to say here. You obviously already made up your mind and I can't make you think other way. So either I fork it and make it my way, or not. Like i said it's an artificial limitation of how the web urls work. There is no spec that says you can't have same names there. My request was purely based on my experience with other web frameworks and some backend as well. Since everywhere we have them distinguished and you don't have them, I get my answer - you want smaller and simpler. Maybe I'm not getting it how much smaller and simpler is it without handling them in the separate collections in the state.

csells commented 2 years ago

well, obviously I can't please everyone with any one API but your argument about the spec not requiring the names to be different on the web is a good one... I'll noodle on it. Should it be params/queryParams, inlineParams/queryParams or something else...? I could leave params alone and have all three... hmmm...

csells commented 2 years ago

I dropped a poll onto Twitter: https://twitter.com/csells/status/1445520767190388738

talamaska commented 2 years ago

:) you're too nice. Maybe I'm not right. Maybe how it is , is for the best. I'm too spoiled from Angular, web dev who may not understand everything that is mobile and honestly not a huge fan of Flutter for Web. If it comes to naming, I would say params and queryParams is the best. Thanks for your time and patience to explain. Feel free to close that issue. Best Regards

P.S will check the poll.

csells commented 2 years ago

hmmm... it looks like the "split it up" proponents are winning the poll. I may need a go_router 2.0 when this is over to make the breaking change clear...

csells commented 2 years ago

fixed in v2.0.0. @talamaska please verify.