Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246
Conversation
ghost
assigned 
| }); | ||
| } else { | ||
| self._store = {}; | ||
| } |
There was a problem hiding this comment.
Seems like it's possible for this code could provide unexpected results for self._store. For example, if url ends with a ?. Also, if all search parameters are integers, then self._store would be an array.
I think it would be safest to initialize it to an empty object in all cases:
self._store = {};
if (searchString) {
urlParams = searchString.split("&");
urlParams.forEach(function (param) {
p = param.split("=");
self._store[decodeURIComponent(p[0])] = decodeURIComponent(p[1]);
});
}
There was a problem hiding this comment.
@redmunds, I believe this is already addressed in the URLParams constructor, which initializes self._store to an empty object.
There was a problem hiding this comment.
If the constructor made the call to parse() and parse() was private method, then I would agree. But parse() is public and can be called multiple times.
There was a problem hiding this comment.
Oops, I missed comment until after my last commit. Let me look into it.
|
Done with review. This looks good. Just a few comments. Thanks for adding unit tests! |
|
@lkcampbell What's the status of this one? I was waiting for you to add a few more unit tests. |
|
@redmunds, I'm going to work on the update this weekend. |
|
@redmunds, changes for code review committed. It occurred to me as I was updating the edge cases that this code doesn't have any exception handling for seriously malformed URLs. Do you want me to address those cases as well or is this sufficient for now? |
Seems like this code should only handle "url params" (hence the name), so I think it's outside of the scope of this module, and don't need to worry about it. |
|
@redmunds, added _store initialization into parse method. Code review changes committed. |
|
Looks good. Merging. |
Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods.
This is a fix for issue #6008. It also adds remove() and isEmpty() methods to the UrlParams module.