feature(urlKeyValue): not rewriting duplicate params in url #2522
Conversation
|
This is like the counterpart to #1921. |
|
|
@joshkurz: |
|
@petebacondarwin yes I have signed the CLA. Its funny. This commit message kinda hints at this being buggy behavior, but your right volta hits it right on the head. |
|
@petebacondarwin Are you able to say when this change will reach master? Thanks, |
|
This week hopefully Pete
|
|
@petebacondarwin That is super! Thanks, |
|
Just looking at this again, we have a bit of a problem when serializing query to a URL: There is no standard way to convert arrays to URL queries. It is server dependent. For instance, in PHP (and Rails?) it expects the param name to be followed by [], e.g. This PR, and others that have been suggested, all want to simply repeat the query parameter in the string. Now this is not too bad since we can simply add the [] to the end of the param name is that is what is required by the server: But then when reading the search you would need to again provide the Is this an acceptable situation? |
|
@petebacondarwin I think we will have a hard time second-guess serialization (and parsing!) behavior of various back-ends. This is why I was proposing to do a (configurable / replacable) service for this. Such a service could be used by both $http and $location. |
|
Hi Pawel. Where did you get to with that? Is this current solution adequate Pete
|
|
@petebacondarwin I didn't implement anything in this respect, was just suggesting this solution in other PRs as the topic of serializing URL params comes back over and over again (as it touches $location, $http and $resource). Your proposal should work but it just feels a bit hacky. I really think that a simple service with 2 methods (serialize, deserialize) would be simple to write and test. It should be an elegant solution. |
|
The service option sounds like a good solution. I assume that a default service would be used? If so, which style would it use? My preference is to use plain param name (i.e. without []). (but I'm not worried as I am happy to swap in my own serialising service.) |
|
@penfold yes, the idea would be to have a default service behaving in a way that code works as today. But people would be able to swap a service at will (and I'm sure that people would come up with services for specific back-ends and publish them to http://ngmodules.org/). |
|
Well actually the current version is broken so we have to change it at least a little. |
|
This PR looks good to me (after all the small stuff I noted is fixed). The serialization schema is consistent with the one used by $http service. I agree that having pluggable serialization mechanism would be better and we can do that in a separate PR. Are there any unresolved design issues that prevent us from merging this in? |
|
Would there be any pushback on exposing the parseKeyValue and toKeyValue methods? They are nice functions that could be used in other instances. |
parseKeyValue and toKeyValue can work with duplicates:
1)parseKeyValue looks for presence of obj[key]
2)detects and replaces obj[key] with [obj[key],val]
3)then pushes more duplicates if neccessary
4)toKeyValue decodes array correctly
5)(not changed)$location.search({param: 'key'}) still replaces if neccessary
6)(not changed)$location.search({param: ['key','key2']}) sets the url with duplicates
BREAKING CHANGE: if the server relied on the buggy behavior then either the
backend should be fixed or a simple serialization of the array should be done
on the client before calling the resource service.
|
just updated the changes requested with a squashed version of the original commit. @penfold @petebacondarwin @pkozlowski-opensource I agree with all of you that a service would be best here as well. Before we go this route, does it make sense to expose the parseKeyValue and toKeyValue methods from the Angular.js file? Then we can convert the logic to a urlUtls service or something along those lines. |
|
@joshkurz I'm somewhat split over the idea. On one side if the methods are made public then it would allow an initial refactor of all the duplicate code for handling keyValues to be made. But on the other hand why not do all the changes in one go? |
|
Ok your right. If we make the methods public, then this PR would extend it's original intent. The purpose here was to add the correct logic for duplicate params. Anything else should be taken care of in a subsequent PR. Thanks ----- Reply message ----- — |
|
Agreed. Also we have think carefully about what we make public API since we have to support it going forward. Better to get this fix in now and then have time to think through the best solution going forward. |
|
Not wanting to sound pushy, but are we good to go with Josh's updated commit? |
|
Totally jumping in out of context here, but could the form fields actually be given names with “[]” in them, like ... If this actually has anything to do with what’s being discussed, then those approaches would turn it into a configuration matter. (I’m also completely new to angular.js, so there’s another reason I may have just misinterpreted the discussion altogether.) On Jun 24, 2013, at 4:33 AM, penfold [email protected] wrote:
|
|
@petebacondarwin @IgorMinar Can this PR be merged into master following @joshkurz's updated commit? |
|
Yep Pete
|
|
@petebacondarwin As this PR is ready, please will you merge it in? (I assume you are the right person to ask?) |
|
Trying to but our CI server has gone a bit pear shaped! Are you running On 1 July 2013 14:48, penfold [email protected] wrote:
|
|
Yes, I'm running off master. ( I got all excited about 1.2 and my delivery date isn't too close. I've got a patched fork but its just the overhead of maintaining it. Looking forward to the next release. |
|
Finally, landed at 8073940! Thanks for the time and comments to move this through. |
|
@petebacondarwin @joshkurz Thanks for getting this fixed. |
parseKeyValue and toKeyValue can work with duplicates:
1) parseKeyValue looks for presence of obj[key]
2) detects and replaces obj[key] with [obj[key],val]
3) then pushes more duplicates if neccessary
4) toKeyValue decodes array correctly
5)(not changed) $location.search({param: 'key'}) still replaces if neccessary
6)(not changed) $location.search({param: ['key','key2']}) sets the url with duplicates
BREAKING CHANGE: the url parameter duplicates are not rewritten anymore.
This will only effect deeplinks. Deeplinks in current apps that depend on
rewriting the duplicates will break. The deeplinks should
be changes to appear how they should look after angular strips the duplicates.