shindig.uri library

A Pure-JS library for URI manipulation.

[mhermanto, 2010-07-21 06:51:07]

http://codereview.appspot.com/1673051/diff/1/4
File features/src/main/javascript/features/shindig.uri/feature.xml (right):

http://codereview.appspot.com/1673051/diff/1/4#newcode25
features/src/main/javascript/features/shindig.uri/feature.xml:25: <container>
Allow gadgets (in a gadget context) to use this feature as a utility class.
Reason not to?

http://codereview.appspot.com/1673051/diff/1/5
File features/src/main/javascript/features/shindig.uri/uri.js (right):

http://codereview.appspot.com/1673051/diff/1/5#newcode28
features/src/main/javascript/features/shindig.uri/uri.js:28: * essentially the
minimum required for various helper routines. Lazy evalution
s/evalution/evaluation/

http://codereview.appspot.com/1673051/diff/1/5#newcode38
features/src/main/javascript/features/shindig.uri/uri.js:38: * + Query and
fragment do not contain their preceding ? and # chars.
What happen if I call setQP() but preceding "?" doesn't work? Will it
automatically add "?" for me? Same with fragment "#".

http://codereview.appspot.com/1673051/diff/1/5#newcode44
features/src/main/javascript/features/shindig.uri/uri.js:44: * var other =  //
resolve() provided in shindig.uri.full
I'm not clear with resolve(), why not just setPath() ?

http://codereview.appspot.com/1673051/diff/1/5#newcode116
features/src/main/javascript/features/shindig.uri/uri.js:116: schema_ ? ":" :
"",
Will empty-string return true?

http://codereview.appspot.com/1673051/diff/1/5#newcode117
features/src/main/javascript/features/shindig.uri/uri.js:117: authority_,
I presume :xxx port numbers are accounted in authority_ as well.

http://codereview.appspot.com/1673051/diff/1/5#newcode133
features/src/main/javascript/features/shindig.uri/uri.js:133: return
str.join('&');
Can we easily drop the very first unnecessary &?

http://codereview.appspot.com/1673051/diff/1/5#newcode137
features/src/main/javascript/features/shindig.uri/uri.js:137: if (str[0] === '?'
|| str[0] === '#') str = str.substr(1);
Nits, consistently use either ' or " throughout this file.

http://codereview.appspot.com/1673051/diff/1/5#newcode145
features/src/main/javascript/features/shindig.uri/uri.js:145: value =
value.replace(/\+/g, " ");
Space needs to be manually unescaped?

http://codereview.appspot.com/1673051/diff/1/5#newcode168
features/src/main/javascript/features/shindig.uri/uri.js:168: return (pmap ||
{})[key];
Neat JS construct.

http://codereview.appspot.com/1673051/diff/1/5#newcode206
features/src/main/javascript/features/shindig.uri/uri.js:206: getQP:
function(key) {
Don't abbreviate method names. It will blend nicer if we have getQueryParams()
and getQueryString(). Similarly elsewhere.

http://codereview.appspot.com/1673051/diff/1/5#newcode221
features/src/main/javascript/features/shindig.uri/uri.js:221: setQP: function()
{
Does this function suppose to take a JSON map of key/vars?

http://codereview.appspot.com/1673051/diff/1/5#newcode225
features/src/main/javascript/features/shindig.uri/uri.js:225: setFP:
function(fpmap) {
fpmap not used.

http://codereview.appspot.com/1673051/diff/1/5#newcode231
features/src/main/javascript/features/shindig.uri/uri.js:231: toString: toString
Suggesting to add setExistingQueryOrFragmentParam(), which will set param in
either query or fragment string when exists. This will be useful for common
container in replacing up_*= and st=, which can be in either cases.

http://codereview.appspot.com/1673051/diff/1/2
File features/src/test/javascript/features/shindig.uri/uritest.js (right):

http://codereview.appspot.com/1673051/diff/1/2#newcode42
features/src/test/javascript/features/shindig.uri/uritest.js:42:
assertEquals("qv2", uri.getQP("qk2"));
Test get[Q|F]P() on non-existent param name. What happen if there are two params
with the same name?

http://codereview.appspot.com/1673051/diff/1/2#newcode59
features/src/test/javascript/features/shindig.uri/uritest.js:59:
assertEquals("www.example.com", uri.getAuthority());
Test what happen when one calls getQuery() and getQP().

http://codereview.appspot.com/1673051/diff/1/2#newcode76
features/src/test/javascript/features/shindig.uri/uritest.js:76:
assertEquals("http", uri.getSchema());
Similarly, test with getFragment() and getFP().

http://codereview.appspot.com/1673051/diff/1/2#newcode195
features/src/test/javascript/features/shindig.uri/uritest.js:195:
assertEquals("//www.example.com/my//path", uri.toString());
nice

http://codereview.appspot.com/1673051/diff/1/2#newcode216
features/src/test/javascript/features/shindig.uri/uritest.js:216:
.setQuery("?one=two&three=four")
I think we should not rely on user prefixing with "?", ie: should just be
setQueryString("one=two&three=four"). Same with fragment.

http://codereview.appspot.com/1673051/diff/1/2#newcode218
features/src/test/javascript/features/shindig.uri/uritest.js:218:
assertEquals("http://www.example.com/my/path?one=two&three=four#five=six");
Where is the actual value being compared to? Same with below tests.

[johnfargo, 2010-07-21 09:24:30]

Hi Michael:

Thanks for the review!

Somehow the code review tool created two of the same code review for this, so
I'm closing this one in favor of the other (which oddly has a much higher ID):
http://codereview.appspot.com/1875044/show

Comments posted here, but the latest patch to 1875044 has all updates.

http://codereview.appspot.com/1673051/diff/1/4
File features/src/main/javascript/features/shindig.uri/feature.xml (right):

http://codereview.appspot.com/1673051/diff/1/4#newcode25
features/src/main/javascript/features/shindig.uri/feature.xml:25: <container>
Nope, adding gadget section.

On 2010/07/21 06:51:07, mhermanto wrote:
> Allow gadgets (in a gadget context) to use this feature as a utility class.
> Reason not to?

http://codereview.appspot.com/1673051/diff/1/5
File features/src/main/javascript/features/shindig.uri/uri.js (right):

http://codereview.appspot.com/1673051/diff/1/5#newcode28
features/src/main/javascript/features/shindig.uri/uri.js:28: * essentially the
minimum required for various helper routines. Lazy evalution
On 2010/07/21 06:51:07, mhermanto wrote:
> s/evalution/evaluation/

Done.

http://codereview.appspot.com/1673051/diff/1/5#newcode38
features/src/main/javascript/features/shindig.uri/uri.js:38: * + Query and
fragment do not contain their preceding ? and # chars.
? and # will be added in uri.toString(); this comment indicates that ? and # are
NOT returned in the getQuery() and getFragment() calls, since in general use
they're far and away more often split off anyway. Deleting comment since it's
confusing: it's not a limitation so much as an API choice.

On 2010/07/21 06:51:07, mhermanto wrote:
> What happen if I call setQP() but preceding "?" doesn't work? Will it
> automatically add "?" for me? Same with fragment "#".

http://codereview.appspot.com/1673051/diff/1/5#newcode44
features/src/main/javascript/features/shindig.uri/uri.js:44: * var other =  //
resolve() provided in shindig.uri.full
Because resolve() handles relative paths:

shindig.uri("http://www.other.com/foo/one/two/three").resolve("bar") -->
http://www.other.com/foo/one/two/bar

Updating comment.

On 2010/07/21 06:51:07, mhermanto wrote:
> I'm not clear with resolve(), why not just setPath() ?

http://codereview.appspot.com/1673051/diff/1/5#newcode116
features/src/main/javascript/features/shindig.uri/uri.js:116: schema_ ? ":" :
"",
No, but this logic is not especially clear. Fixing next up.

On 2010/07/21 06:51:07, mhermanto wrote:
> Will empty-string return true?

http://codereview.appspot.com/1673051/diff/1/5#newcode117
features/src/main/javascript/features/shindig.uri/uri.js:117: authority_,
Correct. Authority includes all of that. If port is desired to be stripped out,
it needs to be done separately.

On 2010/07/21 06:51:07, mhermanto wrote:
> I presume :xxx port numbers are accounted in authority_ as well.

http://codereview.appspot.com/1673051/diff/1/5#newcode133
features/src/main/javascript/features/shindig.uri/uri.js:133: return
str.join('&');
Yes. Fixing in latest update.

On 2010/07/21 06:51:07, mhermanto wrote:
> Can we easily drop the very first unnecessary &?

http://codereview.appspot.com/1673051/diff/1/5#newcode137
features/src/main/javascript/features/shindig.uri/uri.js:137: if (str[0] === '?'
|| str[0] === '#') str = str.substr(1);
I had designs on using ' for single chars, "" for longer strings, but I'm just
going 100% on ".

On 2010/07/21 06:51:07, mhermanto wrote:
> Nits, consistently use either ' or " throughout this file.

http://codereview.appspot.com/1673051/diff/1/5#newcode145
features/src/main/javascript/features/shindig.uri/uri.js:145: value =
value.replace(/\+/g, " ");
I believe I saw this in the unesc method a while back, but to be honest this
particular use of it was copy-and-pasted from other code. This should be
revisited.

On 2010/07/21 06:51:07, mhermanto wrote:
> Space needs to be manually unescaped?

http://codereview.appspot.com/1673051/diff/1/5#newcode168
features/src/main/javascript/features/shindig.uri/uri.js:168: return (pmap ||
{})[key];
:) Sadly, it'll now be gone, w/ changes to maintain param order.

On 2010/07/21 06:51:07, mhermanto wrote:
> Neat JS construct.

http://codereview.appspot.com/1673051/diff/1/5#newcode206
features/src/main/javascript/features/shindig.uri/uri.js:206: getQP:
function(key) {
The point of doing so is to keep the generated JS small. Unlike internal method
names, exported ones cannot be obfuscated. I suppose it's just a few bytes
(here), but when used all over the place gets noticeable. I'm open to change on
this one.

On 2010/07/21 06:51:07, mhermanto wrote:
> Don't abbreviate method names. It will blend nicer if we have getQueryParams()
> and getQueryString(). Similarly elsewhere.

http://codereview.appspot.com/1673051/diff/1/5#newcode221
features/src/main/javascript/features/shindig.uri/uri.js:221: setQP: function()
{
Making this more explicit in the next version. I'll add more JsDoc in a
follow-up. Through the magic of JS, it accepts one of two constructs:
1. setQP("name", "value") --> overrides or sets query param value
2. setQP({ key: "val", other: "again", foo: "bar" }) --> overrides or sets
multiple values at once.

In the interest of API brevity, there is no separate removeParam method. Callers
may set param value to undefined in such case.

On 2010/07/21 06:51:07, mhermanto wrote:
> Does this function suppose to take a JSON map of key/vars?

http://codereview.appspot.com/1673051/diff/1/5#newcode225
features/src/main/javascript/features/shindig.uri/uri.js:225: setFP:
function(fpmap) {
On 2010/07/21 06:51:07, mhermanto wrote:
> fpmap not used.

Done.

http://codereview.appspot.com/1673051/diff/1/5#newcode231
features/src/main/javascript/features/shindig.uri/uri.js:231: toString: toString
Interesting, so catching the either-or case? Seems useful.

I'll add this in util.js -- which you'll notice for now isn't really tested.

I'm still working out the design but my thinking is to include the most minimal
useful Uri parser/builder class possible here, while adding various helper
methods (resolve, setExistingParam, fullyQualify, hasSameOrigin, and others) in
util.js, which may correspond to a new feature (shindig.uri.util) inheriting
from the base.

The idea is to avoid adding the kitchen sink to the base class -- just enough
primitives that others can be added easily.

On 2010/07/21 06:51:07, mhermanto wrote:
> Suggesting to add setExistingQueryOrFragmentParam(), which will set param in
> either query or fragment string when exists. This will be useful for common
> container in replacing up_*= and st=, which can be in either cases.