[1.x] Fix encoded URL

[RobertBoes]

In #592 URL generation was fixed regarding sub paths. However it did introduce encoding issues; #592 (comment)

With this PR it simply decodes the URL to an unencoded variant, which would then be used on the client to set the current location.

[jorqensen]

Please merge this, so I can avoid having to read hieroglyphs in my URL 🙏

[heychazza]

Also facing this bug, becomes very annoying when dealing with dashboards where the URL has a lot of state.

Please merge @reinink especially if 2.0 is dropping soon? Would save a lot of headache

[reinink]

@RobertBoes Hey just to be clear, is the issue here simply that the query params are harder to read because they are encoded? Or is something broken? Just want to make sure I understand the issue.

[RobertBoes]

@reinink I'd say it's broken, not just hard to read. Sure, it doesn't have an impact on the server interpreting the URL, but it still leads to strange behavior.
Let's use the example URL I used in the tests; when you visit that URL in the browser it would navigate to that, so your browser shows /product/123?value=te/st. However, the way Inertia works, it sends back the URL to the client, which is then used to replace the history state, so then it changes to the encoded variant, causing the browser to flash between these URLs. This was never the case with Inertia, this behavior didn't happen before #592.

[pedroborges]

Looks good to me 👍

Before

After

[reinink]

Okay, so I did some testing with this, and I don't think it's quite right yet. I think this encoding is actually important otherwise special characters can mess up your query params.

For example, in my app I have a search feature. Before this change, searching for "jonathan&amy" works great and results in this URL:

/families?search=jonathan%26amy

Here I have a single query param — search.

However, after this change I get this URL instead:

/families?search=jonathan&amy

Which, as you know, means I now have two query params — search and amy.

This is broken as a result of this change, as putting a special character anywhere in your app that ends up in a query param (which is super common) will break someone's app.

So I understand that with the current behavior visiting /families?search=jonathan/amy directly results in a client-side URL update with the encoded version (/families?search=jonathan%2Famy), and I honestly don't know how to fix that without breaking the above mention use-case, nor am I even sure that what we're doing right now is even wrong...maybe the current behavior is actually good?

[jorqensen]

Hey @reinink

Maybe I am not following exactly what you mean, but if I understand correctly...

Which, as you know, means I now have two query params — search and amy.

This is expected behavior with the & sign, when it comes to query params? 🤔

This is broken as a result of this change, as putting a special character anywhere in your app that ends up in a query param (which is super common) will break someone's app.

Surely this is only the & symbol. A query param like:

/families?search=jonathan$amy

Would result in:

['search' => 'jonathan$amy']

To me it sounds like your app relies on the encoding issues introduced in #592, which this PR tries to fix.

If you're worried about breaking existing apps by merging this, I can understand that. However I also think it's a bad idea to call the "bug" a feature, if people do not want encoded URLs. Maybe provide a way to disable it, but leave it on by default?

[RobertBoes]

Chiming in another idea; Let the user provide the URL. So a default implementation that can be overwritten. For example, I don't care about the path prefix, so then I could easily just get the path+query as was done previously. That way I'd send the same URL to the frontend as what's visible in the browser

[heychazza]

I'd even be happy with an option within the Inertia config to specify what characters should be encoded, e.g. &.

Then you've got more control and everyone wins

[reinink]

I'd really like to avoid having to create a config option for something...it feels like something that should be solvable without one.

@RobertBoes @heychazza what's your primary concern here? The fact that history state gets updated with the encoded query params? Or that they are encoded and now visually look worse?

[rojtjo]

Chiming in another idea; Let the user provide the URL. So a default implementation that can be overwritten. For example, I don't care about the path prefix, so then I could easily just get the path+query as was done previously. That way I'd send the same URL to the frontend as what's visible in the browser

I still think this is the best solution! Currently it's quite cumbersome to overwrite the URL generation part.

See #503

[RobertBoes]

@reinink

I'd really like to avoid having to create a config option for something...it feels like something that should be solvable without one.

I totally get that, I wouldn't want that to be "yet another config option", or something like Inertia::parseUrlUsing(...) but more like the way Laravel's internals work. Many classes are bound to the IoC, which allows for extension/modification, with Inertia that's much less the case. That way Inertia implements a default, no need to account for a user-provided callback or anything. The concrete implementation is just loaded from an abstract interface, up to the user if they want to rebind classes.

what's your primary concern here? The fact that history state gets updated with the encoded query params? Or that they are encoded and now visually look worse?

• SEO: The browser loads one URL, then the client updates the URL to a different one; potentially causing issues for multiple URLs
• For me personally when using libraries like spatie/laravel-query-builder it becomes more difficult to debug. Exceptions or reports from users are just a mangled mess of an encoded URL, I can't easily spot issues from the URL

[heychazza]

@reinink

I'd really like to avoid having to create a config option for something...it feels like something that should be solvable without one.

I totally get that, I wouldn't want that to be "yet another config option", or something like Inertia::parseUrlUsing(...) but more like the way Laravel's internals work. Many classes are bound to the IoC, which allows for extension/modification, with Inertia that's much less the case. That way Inertia implements a default, no need to account for a user-provided callback or anything. The concrete implementation is just loaded from an abstract interface, up to the user if they want to rebind classes.

what's your primary concern here? The fact that history state gets updated with the encoded query params? Or that they are encoded and now visually look worse?

• SEO: The browser loads one URL, then the client updates the URL to a different one; potentially causing issues for multiple URLs
• For me personally when using libraries like spatie/laravel-query-builder it becomes more difficult to debug. Exceptions or reports from users are just a mangled mess of an encoded URL, I can't easily spot issues from the URL

Yeah agreed, I'm also using the Spatie Query Builder and it's very hard to understand the URL filters when it's all %20 everything.

Would definitely love if we could just bind our own thing, as I do understand its very niche.