r73093 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r73092‎ | r73093 | r73094 >
Date:22:07, 15 September 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
Added importScriptURI/importStylesheetURI-like functionality.
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -652,15 +652,36 @@
653653 };
654654
655655 /**
656 - * Loads one or more modules for future use
 656+ * Loads an external script or one or more modules for future use
 657+ *
 658+ * @param {mixed} modules either the name of a module, array of modules, or a URL of an external script or style
 659+ * @param {string} type mime-type to use if calling with a URL of an external script or style; acceptable values
 660+ * are "text/css" and "text/javascript"; if no type is provided, text/javascript is assumed
657661 */
658 - this.load = function( modules ) {
 662+ this.load = function( modules, type ) {
659663 // Validate input
660664 if ( typeof modules !== 'object' && typeof modules !== 'string' ) {
661665 throw new Error( 'dependencies must be a string or an array, not a ' + typeof dependencies )
662666 }
663 - // Allow calling with a single dependency as a string
 667+ // Allow calling with an external script or single dependency as a string
664668 if ( typeof modules === 'string' ) {
 669+ // Support adding arbitrary external scripts
 670+ if ( modules.substr( 0, 7 ) == 'http://' || modules.substr( 0, 8 ) == 'https://' ) {
 671+ if ( type === 'text/css' ) {
 672+ setTimeout( function() {
 673+ $( 'head' ).append( '<link rel="stylesheet" type="text/css" href="' + modules + '" />' );
 674+ }, 0 );
 675+ return true;
 676+ } else if ( type === 'text/javascript' || typeof type === 'undefined' ) {
 677+ setTimeout( function() {
 678+ $( 'body' ).append( '<script type="text/javascript" src="' + modules + '"></script>' );
 679+ }, 0 );
 680+ return true;
 681+ }
 682+ // Unknown type
 683+ return false;
 684+ }
 685+ // Called with single module
665686 modules = [modules];
666687 }
667688 // Resolve entire dependency map

Follow-up revisions

RevisionCommit summaryAuthorDate
r74083Improved on r73093 by allowing jQuery to properly escape some HTML attributes.tparscal18:11, 1 October 2010

Comments

#Comment by Catrope (talk | contribs)   15:41, 1 October 2010
+		 * @param {mixed} modules either the name of a module, array of modules, or a URL of an external script or style

You should explicitly document that a URL must start with http(s)://. I'm also not sure that requirement is a good idea, as it doesn't allow for relative or protocol-relative URLs. Relative URLs are also used for fetching normal modules by default (as wgLoadScript is a relative URL by default).

+							$( 'body' ).append( '<script type="text/javascript" src="' + modules + '"></script>' );

You should escape modules properly, e.g. by using jQuery .attr().

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:11, 1 October 2010

We should talk more about the use cases of relative URLs. I'm going to leave it for now because I based this on observations of current usage.

Escaping is fixed in r74083.

Status & tagging log