Decode named route parameters. #2014

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@braddunbar
Collaborator

This patch causes named parameters to be decoded before being passed to their handler as discussed in #1305. Splat parameters are left encoded since they contain more than one uri component. Please note that all parameters to routes created via regular expression will remain encoded.

@jashkenas
Owner

What's the use case for decoding named parts, but not splat parts? Just that you can't distinguish slashes? Seems like a usability problem.

@braddunbar
Collaborator

That's right. Decoding splat parts would prevent splitting on slashes. If it seems like too much of a stretch then I think it's probably fine as is, despite the convenience of leaving out decodeURIComponent where possible in routers.

@philfreo

Big +1 for this patch
Annoying to have to decode the parameters in the normal case
And it's not a big deal to make an exception for splats because people using splats already plan on splitting by slash usually and don't need it decoded in most cases.

@jashkenas
Owner

One quick follow-up before a merge. Folks that are going to use splats are certainly going to want to split on slashes ... but what's the chance that folks using splats are also going to be using slashes as part of their routes? I know sometimes you allow arbitrary user input ... but I doubt that more than 1 or 2% of backbone apps put non-intentional slashes into the splat-part of routes.

Another way to phrase the question -- if we also decoded splat parts, what percent of Backbone apps would encounter problems ... and are there better workarounds for those cases?

@philfreo

Seems you'd never want to split on both %2F and / at the same time (which is why decoding splats always seems like a bad idea) and would lead to unnecessary bugs (although in a small number of cases). If we did decode splat parts, they'd have to do some ugly window.location parsing themselves to workaround those bugs. Even though a small number of people would have both types of slashes, I don't see a reason to turn them into the same thing when they shouldn't be.

@braddunbar
Collaborator

In my experience, splats are used primarily for arbitrary user input since otherwise the parameters would be known ahead of time and named. You would definitely have to parse the splat yourself to work around decoding them.

While the code to accomplish this is a bit awkward, I think that decoding named parameters and not splat parameters is the intent for the vast majority of apps. You can always decode a splat yourself, but you can't re-encode it.

Side Note: You also can't re-encode a named parameter, but I've never had any reason to since all I would care about is slashes. If there is ever a reason to leave named parameters encoded, we should probably just leave them.

@braddunbar
Collaborator

Closing this for reasons similar to those in #2010. Decoding your own parameters isn't particularly difficult and it ensures that Backbone hasn't altered them irreversibly.

@braddunbar braddunbar closed this Dec 28, 2012
@philfreo

What? The reason for closing #2010 is that its double decoding the url. This isn't the case here, right? People now (since 0.9.9) need to manually to decode URL parameters manually then which seems really unnecessary. Annoying extra line per param in every route. Seems like no good reason not to merge this.

@braddunbar
Collaborator

People would always need to decode URL parameters manually then which seems really unnecessary.

I would like to verify this assertion before proceeding. If there is any reason that the user might want to keep the parameter encoded, then we should leave it be as the operation is not reversible. That said, perhaps you would just use a splat parameter in that case? I'll likely reopen after some thought, but I'm not quite sure about this yet.

@jashkenas jashkenas reopened this Dec 29, 2012
@philfreo

Any more thoughts on this? Not to beat a dead horse but I really think you shouldn't have to manually decode parameters in the normal case.

@braddunbar
Collaborator

@philfreo I agree. It seems reasonable to have both (:param is decoded, *param is not). I'll clean this up and update the pull request later today. I've been reading a bit and can't see any reason not to.

@philfreo

bump :)

@braddunbar
Collaborator

I should never mention a specific time in my comments. It nearly always guarantees that I won't have time for it. :)

Sorry about the delay @philfreo. I'll clean this up as soon as I can.

@jashkenas
Owner

@braddunbar -- if you do have the time to clean this up, I'd love to merge it.

@jashkenas jashkenas closed this in d6e283e Mar 20, 2013
@edgarRd

The current state of the router decodes all parameters found (https://github.com/documentcloud/backbone/blob/master/backbone.js#L1293). I was wondering if it could be posible (upon discussion) to add an option to the router to deactivate this at user will. I think delegating URL parameter processing to other pieces of code in the system should be a valid use case for this functionality.

A simple example, lets say that I'm catching all routes using just one function to delegate the route processing to other entities using something like this:

routes: {
  "*routes": "router"
},

router: function (args) {
    args = (args) ? args.split('/') : ["!","home"];

    var routeArgs = Array.prototype.slice.call(args);
    console.log("args in route: "+ routeArgs);

    // publish the route for processing by entity X
    processMyRoute(routeArgs);
}

Given the fact that the current router decodes everything, if I have some complex parameters such as URIs in the URL fragment (or other type of free text encoded there), this can't be done straightforward with the current implementation.

I don't think it'll be much trouble to add a parameter to switch on/off the decoding (maybe having the current behavior as default), and just let the user decide if they want to decode or not their parameters.

Thanks.

@braddunbar
Collaborator

I've run into this recently as well. I've been using a fragment like users/q:query+key:value+key:value and decoding by default throws away encoding of keys/values. I don't know that we can satisfy both ends of this issue, but decoding by default is destructive while leaving values alone is not.

@edgarRd

I think we should not sacrifice flexibility for convenience on this issue, so I think that adding an option to trigger the functionality (in either way) should address both objectives.
If users don't want to add an extra line of code for decoding params on every route handler they just activate the encoding or whatever the default is, but that doesn't throw away the flexibility of doing whatever you want with the encoded parameters.

@philfreo

@edgarRd I thought the current router wasn't decoding splats like *routes, so you should be good?

@braddunbar
Collaborator

@philfreo Nah, it decodes everything. I would be in favor of removing the decoding from splats, but the code is rather ugly.

@philfreo

Assuming there is a halfway reasonable implementation possible, seems like that (decoding only splats) is much better solution than a) adding a config variable, b) making users decode manually in all cases, or c) losing data

@edgarRd

@philfreo Could you elaborate on why only splats is much better? @braddunbar made a reference to the case having a fragment with params (users/q:query+key:value+key:value) in which the current implementation throws away the encoding (assuming this is not the desired behavior in his use case of course), but I can see how this could be used in other settings where you may need the encoded params too.

@philfreo

@edgarRd because in almost all cases without splats, it's annoying for users to have to decode them in the regular case (and backbone has never required the simple case to be manually decoded, except for during a couple short-lived versions)

Either you have simple parameters, or you are doing something really complex where you want to decode it yourself. In the second case, you can use splats. (And if you really didn't want to allow slashes, you could handle that manually since it means you're already doing custom parsing of the parameter)

@edgarRd

@philfreo thanks for the explanation. I agree, not decoding splats would be a good improvement in the flexibility keeping the convenience of simple params decoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment