fix for ac.url.make weird behavior after micro-optmiziation #854

Merged
merged 6 commits into from Dec 12, 2012

Projects

None yet

2 participants

@caridy
Collaborator
caridy commented Dec 11, 2012
  • ac.url.make issues
  • func tests failures after the first iteration of the optimization
@drewfish drewfish commented on an outdated diff Dec 11, 2012
lib/app/addons/ac/url.common.js
if (urlParams) {
- urlParams = objectToQueryStr(urlParams, true);
-
- if (urlParams && urlParams.length) {
- url = url + '?' + urlParams;
+ if (Y.Lang.isObject(urlParams)) {
@drewfish
drewfish Dec 11, 2012 Member

I think Y.Lang.isObject() returns true for arrays too, although it probably doesn't matter too much for this situation.

@drewfish drewfish commented on an outdated diff Dec 12, 2012
lib/app/addons/ac/url.common.js
if (urlParams) {
- urlParams = objectToQueryStr(urlParams, true);
-
- if (urlParams && urlParams.length) {
- url = url + '?' + urlParams;
+ if (Y.Lang.isObject(urlParams)) {
+ // adding querystring params to routeParams and let
+ // the url maker to create the proper url. Empty params
+ // will be left out. TODO: why?
+ for (key in urlParams) {
+ if (urlParams.hasOwnProperty(key) && urlParams[key]) {
+ params[key] = urlParams[key];
+ }
+ }
+ } else {
+ Y.log('ac.url.make is expecting an object as urlParams instead of: ' + urlParams, 'warn', NAME);
@drewfish
drewfish Dec 12, 2012 Member

Small point: perhaps Y.JSON.stringify(urlParams) just to be safe.

@drewfish
Member

A couple of small comments. +1 even without those addressed.

@caridy
Collaborator
caridy commented Dec 12, 2012

@drewfish, can you do a second pass on this one?

@drewfish drewfish commented on the diff Dec 12, 2012
lib/app/addons/ac/url.common.js
if (urlParams) {
- urlParams = objectToQueryStr(urlParams, true);
-
- if (urlParams && urlParams.length) {
- url = url + '?' + urlParams;
+ urlParams = (typeof urlParams === 'string' ? Y.QueryString.parse(urlParams) :
+ urlParams);
+
+ // adding querystring params to routeParams and let
+ // the url maker to create the proper url. Empty params
+ // will be left out. TODO: why?
@drewfish
drewfish Dec 12, 2012 Member

I think empty params are left out to "optimize" the URL -- so that it's as short as possible.

@drewfish
Member

+1

@caridy caridy merged commit 7c48652 into yahoo:develop Dec 12, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment