Links include default param in url in 0.24

[confact]

I noticed that after I upgraded to 0.24 it started including the query in links even if they are not really set and should use the default values.

This is deployed in production on Kindmetrics as it does work but doesn't look nice. You can see at the demo:
https://app.kindmetrics.io/share/851Jv1PW

Click on pages or sources and you can see the URL have all queries with default values.

I use this code to generate the URLs (which worked prev 0.24):

Domains::Show.with(domain_id: domain.id, goal_id: goal.try { |g| g.id } || 0_i64, site_path: row.address.to_s, source_name: source_name, medium_name: medium_name, from: time_to_string(from), to: time_to_string(to)).url

This would be nice to get fixed as it doesn't look nice :)

[jwoertink]

Actually, that was something @stephendolan requested and @matthewmcgarvey made a PR for 😅 #1201

So I guess the question is... which way do we want it? I can't think of a way where we can have both 🤔

[confact]

@jwoertink hehe, typical.

Why I want to fill parameters even if it defaults is to make it easier to handle fallback if it is not filled. Now I would need to do if statement for every parameter to check if it is default value.

How it worked before was nice as I didn't need to think about that.

I think it seems to be good to use in different situations.
Maybe @paulcsmith could give his thoughts on this.

[confact]

maybe @stephendolan and @matthewmcgarvey have some ideas as well? Would love to discuss how we can solve this is the best way :)

Otherwise, I will just fork and revert #1201

[matthewmcgarvey]

I agree that in your case it's not a clean url, but I don't know of any examples where it wouldn't look like that in another framework. Looking into kindmetrics source, I see that most of the issue is coming from https://github.com/kindmetrics/kindmetrics/blob/66cffa4042a8c9201c1d601a5447d83cb3d95371/src/pages/domains/show_page.cr#L150-L158

Maybe instead of setting the default value if it is missing you could set it to nil. By making it nil it won't be added to the generated url. Would that work?

[confact]

@matthewmcgarvey those are only used for the tabs (filters) when clicking on the page URL or sources to filter it, you will get a tab on top left corner. These are used there. I can fix these at least with nil. So it solves one part of the puzzle.

On the date picker dropdown and on all links, it won't work. Those would need to add if statements as they don't use it.
as an example: https://github.com/kindmetrics/kindmetrics/blob/66cffa4042a8c9201c1d601a5447d83cb3d95371/src/components/period_dropdown_component.cr#L64
And: https://github.com/kindmetrics/kindmetrics/blob/66cffa4042a8c9201c1d601a5447d83cb3d95371/src/pages/domains/data/source_page.cr#L54

[matthewmcgarvey]

Would making the needs nilable rather than default to blank strings help? https://github.com/kindmetrics/kindmetrics/blob/66cffa4042a8c9201c1d601a5447d83cb3d95371/src/components/period_dropdown_component.cr#L9-L11

[stephendolan]

Yeah, my original submission (thinking it was a bug) mainly stemmed from a lot of confusion about why things I was explicitly passing seemed to be disappearing since other frameworks I've used didn't handle things that way.

I do still think that going with the implementation of "least surprise" that we're at now seems nicer from a framework perspective, but if we can find a way to eject from that default and filter them from the URL more elegantly I'm certainly in favor.

[edwardloveall]

As an example: I think UserDashboard::Index.with(section: "stats") should resolve to /user_dashboards?section=stats and UserDashboard::Index.path should resolve to /user_dashboards, regardless of action defaults. A slightly more ambiguous case resolves the same way: SearchResults.with(query: "") should resolve to /results?query= because that "" is meaningful. This feels like the principle of lease surprise, as @stephendolan said.

Granted, it can lead to some ugly URLs, but since the code asked for those parts of the URL explicitly, that feels correct. And I completely understand @confact's problem here that a page necessarily needs to know what parameterized data to display and those parameters have to come from somewhere. It's a difficult problem to solve with only a route object.

Could a solution be to push the problem up to the param level instead of trying to solve it in the route level? Instead of having multiple needs in the component, declare a single needs of type PeriodDropdownParams that represents all the params possible for that component. A build method could then:

• Look at all the parameters passed in during new
• Have default way to render each parameter (or not if it's the default)
• Build up the needed parameters without the values that use the defaults which is then finally passed to the route

I think this allows:

• No one to be surprised by what parameters are generated where
• No ugly URLs when there parameters are not adding any value

Thoughts?

[confact]

@edwardloveall that sounds like a good solution in the right direction. I am not totally sold on having one object handling all params. but indeed sound like a good start point.

Maybe have the object in the background and let it be controller by macro methods to set the settings could be a good way?

@matthewmcgarvey thank you for trying to help me fix the issues. I have fixed most of the issues which were due to empty strings here: kindmetrics/kindmetrics#106 - But it still doesn't act the way I want on to and from and period due to default value. I would need to add checks on the period on the links there if the period is the default value. Otherwise, it is okay, 50 lines more code, and still works almost the same.

I really did like that the URL generator took care of my default values for me. That was actually one of the things that made me go wow when coding https://github.com/microgit-com/microgit and Kindmetrics.

This does feel like a personal touch/interest. And I understand I am the minority here.

So you can close this and I have to find ways to figure out my issues with default values on period.

[paulcsmith]

Just seeing this now. I do think the current behavior is the "least surprising" but I do get what you mean @confact. I think ideally we keep the current behavior as-is, but add an option for the param macro that allows you to hide the defaults

param show_something : Bool = true, hide_defaults_from_params: true

This is a horrible option name, but I think this could make it so people can customize the handling of params. I think this is a lower priority since there are lots of other feature, but this could be an option if the code isn't too crazy. Thoughts?

[confact]

@paulcsmith I understand that for the majority it is easier as it is now. So I am okay with that.

I like this setting to the param macro. the option name could be:

param show_something : Bool = true, hide_default: true

it should maybe default to false - making shorter is good and it still says what it does - hiding the default value in the param in the URL - we can also explain more in detail what it does in the documentation.

So in summary, I do like this more. I hope @edwardloveall, @matthewmcgarvey, and @jwoertink agrees :)