r99444 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r99443‎ | r99444 | r99445 >
Date:23:15, 10 October 2011
Author:neilk
Status:ok (Comments)
Tags:todo 
Comment:
protocol-relative URLs for javascript. Could also be extended to relative urls of any kind -- now mw.Uri is just "relative" to the current URL
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.Uri.js (modified) (history)
  • /trunk/phase3/tests/jasmine/spec/mediawiki.Uri.spec.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.Uri.js
——@@ -56,7 +56,7 @@
5757 *
5858 */
5959
60 -( function( $ ) {
 60+( function( $, mw ) {
6161
6262 /**
6363 * Function that's useful when constructing the URI string -- we frequently encounter the pattern of
——@@ -93,168 +93,191 @@
9494 'fragment' // top
9595 ];
9696
97 - /**
98 - * Constructs URI object. Throws error if arguments are illegal/impossible, or otherwise don't parse.
99 - * @constructor
100 - * @param {!Object|String} URI string, or an Object with appropriate properties (especially another URI object to clone). Object must have non-blank 'protocol', 'host', and 'path' properties.
101 - * @param {Boolean} strict mode (when parsing a string)
102 - */
103 - mw.Uri = function( uri, strictMode ) {
104 - strictMode = !!strictMode;
105 - if ( uri !== undefined && uri !== null || uri !== '' ) {
106 - if ( typeof uri === 'string' ) {
107 - this._parse( uri, strictMode );
108 - } else if ( typeof uri === 'object' ) {
109 - var _this = this;
110 - $.each( properties, function( i, property ) {
111 - _this[property] = uri[property];
112 - } );
113 - if ( this.query === undefined ) {
114 - this.query = {};
115 - }
116 - }
117 - }
118 - if ( !( this.protocol && this.host && this.path ) ) {
119 - throw new Error( 'Bad constructor arguments' );
120 - }
121 - };
12297
12398 /**
124 - * Standard encodeURIComponent, with extra stuff to make all browsers work similarly and more compliant with RFC 3986
125 - * Similar to rawurlencode from PHP and our JS library mw.util.rawurlencode, but we also replace space with a +
126 - * @param {String} string
127 - * @return {String} encoded for URI
 99+ * We use a factory to inject a document location, for relative URLs, including protocol-relative URLs.
 100+ * so the library is still testable & purely functional.
128101 */
129 - mw.Uri.encode = function( s ) {
130 - return encodeURIComponent( s )
131 - .replace( /!/g, '%21').replace( /'/g, '%27').replace( /\(/g, '%28')
132 - .replace( /\)/g, '%29').replace( /\*/g, '%2A')
133 - .replace( /%20/g, '+' );
134 - };
 102+ mw.UriRelative = function( documentLocation ) {
135103
136 - /**
137 - * Standard decodeURIComponent, with '+' to space
138 - * @param {String} string encoded for URI
139 - * @return {String} decoded string
140 - */
141 - mw.Uri.decode = function( s ) {
142 - return decodeURIComponent( s ).replace( /\+/g, ' ' );
143 - };
144 -
145 - mw.Uri.prototype = {
146 -
147104 /**
148 - * Parse a string and set our properties accordingly.
149 - * @param {String} URI
150 - * @param {Boolean} strictness
151 - * @return {Boolean} success
 105+ * Constructs URI object. Throws error if arguments are illegal/impossible, or otherwise don't parse.
 106+ * @constructor
 107+ * @param {!Object|String} URI string, or an Object with appropriate properties (especially another URI object to clone). Object must have non-blank 'protocol', 'host', and 'path' properties.
 108+ * @param {Boolean} strict mode (when parsing a string)
152109 */
153 - _parse: function( str, strictMode ) {
154 - var matches = parser[ strictMode ? 'strict' : 'loose' ].exec( str );
155 - var uri = this;
156 - $.each( properties, function( i, property ) {
157 - uri[ property ] = matches[ i+1 ];
158 - } );
159 -
160 - // uri.query starts out as the query string; we will parse it into key-val pairs then make
161 - // that object the "query" property.
162 - // we overwrite query in uri way to make cloning easier, it can use the same list of properties.
163 - var q = {};
164 - // using replace to iterate over a string
165 - if ( uri.query ) {
166 - uri.query.replace( /(?:^|&)([^&=]*)(?:(=)([^&]*))?/g, function ($0, $1, $2, $3) {
167 - if ( $1 ) {
168 - var k = mw.Uri.decode( $1 );
169 - var v = ( $2 === '' || $2 === undefined ) ? null : mw.Uri.decode( $3 );
170 - if ( typeof q[ k ] === 'string' ) {
171 - q[ k ] = [ q[ k ] ];
172 - }
173 - if ( typeof q[ k ] === 'object' ) {
174 - q[ k ].push( v );
175 - } else {
176 - q[ k ] = v;
177 - }
 110+ function Uri( uri, strictMode ) {
 111+ strictMode = !!strictMode;
 112+ if ( uri !== undefined && uri !== null || uri !== '' ) {
 113+ if ( typeof uri === 'string' ) {
 114+ this._parse( uri, strictMode );
 115+ } else if ( typeof uri === 'object' ) {
 116+ var _this = this;
 117+ $.each( properties, function( i, property ) {
 118+ _this[property] = uri[property];
 119+ } );
 120+ if ( this.query === undefined ) {
 121+ this.query = {};
178122 }
179 - } );
 123+ }
180124 }
181 - this.query = q;
182 - },
183125
184 - /**
185 - * Returns user and password portion of a URI.
186 - * @return {String}
187 - */
188 - getUserInfo: function() {
189 - return cat( '', this.user, cat( ':', this.password, '' ) );
190 - },
 126+ // protocol-relative URLs
 127+ if ( !this.protocol ) {
 128+ this.protocol = defaultProtocol;
 129+ }
191130
 131+ if ( !( this.protocol && this.host && this.path ) ) {
 132+ throw new Error( 'Bad constructor arguments' );
 133+ }
 134+ }
 135+
192136 /**
193 - * Gets host and port portion of a URI.
194 - * @return {String}
 137+ * Standard encodeURIComponent, with extra stuff to make all browsers work similarly and more compliant with RFC 3986
 138+ * Similar to rawurlencode from PHP and our JS library mw.util.rawurlencode, but we also replace space with a +
 139+ * @param {String} string
 140+ * @return {String} encoded for URI
195141 */
196 - getHostPort: function() {
197 - return this.host + cat( ':', this.port, '' );
198 - },
 142+ Uri.encode = function( s ) {
 143+ return encodeURIComponent( s )
 144+ .replace( /!/g, '%21').replace( /'/g, '%27').replace( /\(/g, '%28')
 145+ .replace( /\)/g, '%29').replace( /\*/g, '%2A')
 146+ .replace( /%20/g, '+' );
 147+ };
199148
200149 /**
201 - * Returns the userInfo and host and port portion of the URI.
202 - * In most real-world URLs, this is simply the hostname, but it is more general.
203 - * @return {String}
 150+ * Standard decodeURIComponent, with '+' to space
 151+ * @param {String} string encoded for URI
 152+ * @return {String} decoded string
204153 */
205 - getAuthority: function() {
206 - return cat( '', this.getUserInfo(), '@' ) + this.getHostPort();
207 - },
 154+ Uri.decode = function( s ) {
 155+ return decodeURIComponent( s ).replace( /\+/g, ' ' );
 156+ };
208157
209 - /**
210 - * Returns the query arguments of the URL, encoded into a string
211 - * Does not preserve the order of arguments passed into the URI. Does handle escaping.
212 - * @return {String}
213 - */
214 - getQueryString: function() {
215 - var args = [];
216 - $.each( this.query, function( key, val ) {
217 - var k = mw.Uri.encode( key );
218 - var vals = val === null ? [ null ] : $.makeArray( val );
219 - $.each( vals, function( i, v ) {
220 - args.push( k + ( v === null ? '' : '=' + mw.Uri.encode( v ) ) );
 158+ Uri.prototype = {
 159+
 160+ /**
 161+ * Parse a string and set our properties accordingly.
 162+ * @param {String} URI
 163+ * @param {Boolean} strictness
 164+ * @return {Boolean} success
 165+ */
 166+ _parse: function( str, strictMode ) {
 167+ var matches = parser[ strictMode ? 'strict' : 'loose' ].exec( str );
 168+ var uri = this;
 169+ $.each( properties, function( i, property ) {
 170+ uri[ property ] = matches[ i+1 ];
221171 } );
222 - } );
223 - return args.join( '&' );
224 - },
225172
226 - /**
227 - * Returns everything after the authority section of the URI
228 - * @return {String}
229 - */
230 - getRelativePath: function() {
231 - return this.path + cat( '?', this.getQueryString(), '', true ) + cat( '#', this.fragment, '' );
232 - },
 173+ // uri.query starts out as the query string; we will parse it into key-val pairs then make
 174+ // that object the "query" property.
 175+ // we overwrite query in uri way to make cloning easier, it can use the same list of properties.
 176+ var q = {};
 177+ // using replace to iterate over a string
 178+ if ( uri.query ) {
 179+ uri.query.replace( /(?:^|&)([^&=]*)(?:(=)([^&]*))?/g, function ($0, $1, $2, $3) {
 180+ if ( $1 ) {
 181+ var k = Uri.decode( $1 );
 182+ var v = ( $2 === '' || $2 === undefined ) ? null : Uri.decode( $3 );
 183+ if ( typeof q[ k ] === 'string' ) {
 184+ q[ k ] = [ q[ k ] ];
 185+ }
 186+ if ( typeof q[ k ] === 'object' ) {
 187+ q[ k ].push( v );
 188+ } else {
 189+ q[ k ] = v;
 190+ }
 191+ }
 192+ } );
 193+ }
 194+ this.query = q;
 195+ },
233196
234 - /**
235 - * Gets the entire URI string. May not be precisely the same as input due to order of query arguments.
236 - * @return {String} the URI string
237 - */
238 - toString: function() {
239 - return this.protocol + '://' + this.getAuthority() + this.getRelativePath();
240 - },
 197+ /**
 198+ * Returns user and password portion of a URI.
 199+ * @return {String}
 200+ */
 201+ getUserInfo: function() {
 202+ return cat( '', this.user, cat( ':', this.password, '' ) );
 203+ },
241204
242 - /**
243 - * Clone this URI
244 - * @return {Object} new URI object with same properties
245 - */
246 - clone: function() {
247 - return new mw.Uri( this );
248 - },
 205+ /**
 206+ * Gets host and port portion of a URI.
 207+ * @return {String}
 208+ */
 209+ getHostPort: function() {
 210+ return this.host + cat( ':', this.port, '' );
 211+ },
249212
250 - /**
251 - * Extend the query -- supply query parameters to override or add to ours
252 - * @param {Object} query parameters in key-val form to override or add
253 - * @return {Object} this URI object
254 - */
255 - extend: function( parameters ) {
256 - $.extend( this.query, parameters );
257 - return this;
258 - }
 213+ /**
 214+ * Returns the userInfo and host and port portion of the URI.
 215+ * In most real-world URLs, this is simply the hostname, but it is more general.
 216+ * @return {String}
 217+ */
 218+ getAuthority: function() {
 219+ return cat( '', this.getUserInfo(), '@' ) + this.getHostPort();
 220+ },
 221+
 222+ /**
 223+ * Returns the query arguments of the URL, encoded into a string
 224+ * Does not preserve the order of arguments passed into the URI. Does handle escaping.
 225+ * @return {String}
 226+ */
 227+ getQueryString: function() {
 228+ var args = [];
 229+ $.each( this.query, function( key, val ) {
 230+ var k = Uri.encode( key );
 231+ var vals = val === null ? [ null ] : $.makeArray( val );
 232+ $.each( vals, function( i, v ) {
 233+ args.push( k + ( v === null ? '' : '=' + Uri.encode( v ) ) );
 234+ } );
 235+ } );
 236+ return args.join( '&' );
 237+ },
 238+
 239+ /**
 240+ * Returns everything after the authority section of the URI
 241+ * @return {String}
 242+ */
 243+ getRelativePath: function() {
 244+ return this.path + cat( '?', this.getQueryString(), '', true ) + cat( '#', this.fragment, '' );
 245+ },
 246+
 247+ /**
 248+ * Gets the entire URI string. May not be precisely the same as input due to order of query arguments.
 249+ * @return {String} the URI string
 250+ */
 251+ toString: function() {
 252+ return this.protocol + '://' + this.getAuthority() + this.getRelativePath();
 253+ },
 254+
 255+ /**
 256+ * Clone this URI
 257+ * @return {Object} new URI object with same properties
 258+ */
 259+ clone: function() {
 260+ return new Uri( this );
 261+ },
 262+
 263+ /**
 264+ * Extend the query -- supply query parameters to override or add to ours
 265+ * @param {Object} query parameters in key-val form to override or add
 266+ * @return {Object} this URI object
 267+ */
 268+ extend: function( parameters ) {
 269+ $.extend( this.query, parameters );
 270+ return this;
 271+ }
 272+ };
 273+
 274+ var defaultProtocol = ( new Uri( documentLocation ) ).protocol;
 275+
 276+ return Uri;
259277 };
260278
261 -} )( jQuery );
 279+ // inject the current document location, for relative URLs
 280+ mw.Uri = mw.UriRelative( document.location.href );
 281+
 282+
 283+
 284+} )( jQuery, mediaWiki );
Index: trunk/phase3/tests/jasmine/spec/mediawiki.Uri.spec.js
——@@ -7,7 +7,7 @@
88 function basicTests( strict ) {
99
1010 describe( "should parse a simple HTTP URI correctly", function() {
11 -
 11+
1212 var uriString = 'http://www.ietf.org/rfc/rfc2396.txt';
1313 var uri;
1414 if ( strict ) {
——@@ -238,6 +238,16 @@
239239
240240 } );
241241
 242+ describe( "should handle protocol-relative URLs", function() {
 243+
 244+ it ( "should create protocol-relative URLs with same protocol as document", function() {
 245+ var uriRel = mw.UriRelative( 'glork://en.wiki.local/foo.php' );
 246+ var uri = new uriRel( '//en.wiki.local/w/api.php' );
 247+ expect( uri.protocol ).toEqual( 'glork' );
 248+ } );
 249+
 250+ } );
 251+
242252 it( "should throw error on no arguments to constructor", function() {
243253 expect( function() {
244254 var uri = new mw.Uri();
——@@ -262,12 +272,17 @@
263273 } ).toThrow( "Bad constructor arguments" );
264274 } );
265275
266 - it( "should throw error on URI without protocol as argument to constructor", function() {
 276+ it( "should throw error on URI without protocol or // in strict mode", function() {
267277 expect( function() {
268 - var uri = new mw.Uri( 'foo.com/bar/baz' );
 278+ var uri = new mw.Uri( 'foo.com/bar/baz', true );
269279 } ).toThrow( "Bad constructor arguments" );
270280 } );
271281
 282+ it( "should normalize URI without protocol or // in loose mode", function() {
 283+ var uri = new mw.Uri( 'foo.com/bar/baz', false );
 284+ expect( uri.toString() ).toEqual( 'http://foo.com/bar/baz' );
 285+ } );
 286+
272287 } );
273288
274289 } )();

Comments

#Comment by NeilK (talk | contribs)   23:21, 10 October 2011

The diff is confusing here, might be better to view without space changes: 

$ svn diff -x -b -c 99444

Essentially the whole Uri class now is moved inside of UriRelative.

UriRelative() takes a URL, and returns a Uri parser which will be "relative" to that URL.

mw.Uri is now just the URI parser which is relative to the current document location.

This is done this way for testability, as you see we can inject a location into the parser without actually needing the browser functionality of document.location.href. Also we might one day want to calculate relative URLs this way, perhaps with the host, and with /../ in the path and so on.

#Comment by Raindrift (talk | contribs)   17:35, 31 October 2011

Just a minor thing, it could be a little more clear what's going on here: 

if ( uri !== undefined && uri !== null || uri !==  ) {
#Comment by NeilK (talk | contribs)   00:44, 30 November 2011

you know, I'm not sure what I meant by that. Shouldn't it be an && ?

#Comment by Krinkle (talk | contribs)   00:38, 30 November 2011

Diff confusing indeed, ViewVC link on top is fine though (it ignores whitespace by default). 

-	mw.Uri = function( uri, strictMode ) {
+		function Uri( uri, strictMode ) {
+		return Uri;

This means mw.Uri is no longer a public object constructor that can be extended. Or does it ?

Oh I see, the mw.UriRelative is a factory-like function using the (now private) object constructor as base to create new object constructors.

Idea: To allow future extendability, perhaps expose this base constructor as well so that its prototype is accessible. Would also be nice if the created object constructor would extend/inherit from the base constructor, so that everything is inherited / re-used by reference (instead of the constructor and all prototypes being re-created on every call to mw.UriRelative, albeit that mw.UriRelative is likely only called once or twice on a regular page).

#Comment by NeilK (talk | contribs)   00:48, 30 November 2011

No, it's all the same as it was (note all the tests still pass). Think of mw.UriRelative an object that returns a constructor for URIs. So mw.Uri is still a constructor.

I believe that it's just as extensible as it was before.

#Comment by Krinkle (talk | contribs)   01:22, 30 November 2011

Yeah, it's fine as it is. There is no regression, it's an improvement.

But now that there will be more than one object constructor for Uri's (fabricated by mw.UriRelative) it would be clean to be theoretically able to alter the prototype for all or some of them at any point in time (which is what makes prototypes so cool, due to the inheriting nature, adding one later affects all). However it's a non-issue at this point, just an idea.

#Comment by NeilK (talk | contribs)   01:15, 30 November 2011

Also, unfortunately for performance, all of Uri becomes dependent on the environment, but that's the whole point of this change.

I might be able to stash the Uri.prototype outside of the function, and then just say that Uri.prototype = thatPrototype in the UriRelative constructor. But I think for a smart compiler that won't really make much of a difference.

Status & tagging log