Skip to content

Commit

Permalink
Fix #13355. Tweak Uglify options and var order for gzip. Close gh-1151.
Browse files Browse the repository at this point in the history 
Change uglify-js options for compressor
Change variables initialization sequence for some declarations
  • Loading branch information
@markelog @dmethvin
markelog authored and dmethvin committed Jan 31, 2013
1 parent e392e55 commit d79bf35
Show file tree
Hide file tree
Showing 12 changed files with 63 additions and 57 deletions.
6 changes: 6 additions & 0 deletions Gruntfile.js
View file
Expand Up @@ -101,6 +101,12 @@ module.exports = function( grunt ) {
options: { options: {
banner: "/*! jQuery v<%= pkg.version %> | (c) 2005, 2012 jQuery Foundation, Inc. | jquery.org/license */", banner: "/*! jQuery v<%= pkg.version %> | (c) 2005, 2012 jQuery Foundation, Inc. | jquery.org/license */",
sourceMap: "dist/jquery.min.map", sourceMap: "dist/jquery.min.map",
compress: {
hoist_funs: false,
join_vars: false,
loops: false,
unused: false
},
beautify: { beautify: {
ascii_only: true ascii_only: true
} }
Expand Down
28 changes: 14 additions & 14 deletions src/ajax.js
View file
Expand Up @@ -2,7 +2,6 @@ var
// Document location // Document location
ajaxLocParts, ajaxLocParts,
ajaxLocation, ajaxLocation,

ajax_nonce = jQuery.now(), ajax_nonce = jQuery.now(),


ajax_rquery = /\?/, ajax_rquery = /\?/,
Expand Down Expand Up @@ -115,7 +114,7 @@ function inspectPrefiltersOrTransports( structure, options, originalOptions, jqX
// that takes "flat" options (not to be deep extended) // that takes "flat" options (not to be deep extended)
// Fixes #9887 // Fixes #9887
function ajaxExtend( target, src ) { function ajaxExtend( target, src ) {
var key, deep, var deep, key,
flatOptions = jQuery.ajaxSettings.flatOptions || {}; flatOptions = jQuery.ajaxSettings.flatOptions || {};


for ( key in src ) { for ( key in src ) {
Expand All @@ -135,7 +134,7 @@ jQuery.fn.load = function( url, params, callback ) {
return _load.apply( this, arguments ); return _load.apply( this, arguments );
} }


var selector, type, response, var selector, response, type,
self = this, self = this,
off = url.indexOf(" "); off = url.indexOf(" ");


Expand Down Expand Up @@ -316,20 +315,23 @@ jQuery.extend({
// Force options to be an object // Force options to be an object
options = options || {}; options = options || {};


var transport, var // Cross-domain detection vars
parts,
// Loop variable
i,
// URL without anti-cache param // URL without anti-cache param
cacheURL, cacheURL,
// Response headers // Response headers as string
responseHeadersString, responseHeadersString,
responseHeaders,
// timeout handle // timeout handle
timeoutTimer, timeoutTimer,
// Cross-domain detection vars
parts,
// To know if global events are to be dispatched // To know if global events are to be dispatched
fireGlobals, fireGlobals,
// Loop variable
i, transport,
// Response headers
responseHeaders,

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jan 31, 2013

Member

So, this is what I was talking about: related variables separated (responseHeaders, responseHeadersString). Reason? Unknown unless you dig down the commit history.

The whole declaration part is a mess now because it's impossible to determine what belongs together.
// Create the final options object // Create the final options object
s = jQuery.ajaxSetup( {}, options ), s = jQuery.ajaxSetup( {}, options ),
// Callbacks context // Callbacks context
Expand Down Expand Up @@ -704,8 +706,7 @@ jQuery.extend({
* - returns the corresponding response * - returns the corresponding response
*/ */
function ajaxHandleResponses( s, jqXHR, responses ) { function ajaxHandleResponses( s, jqXHR, responses ) {

var firstDataType, ct, finalDataType, type,
var ct, type, finalDataType, firstDataType,
contents = s.contents, contents = s.contents,
dataTypes = s.dataTypes, dataTypes = s.dataTypes,
responseFields = s.responseFields; responseFields = s.responseFields;
Expand Down Expand Up @@ -766,8 +767,7 @@ function ajaxHandleResponses( s, jqXHR, responses ) {


// Chain conversions given the request and the original response // Chain conversions given the request and the original response
function ajaxConvert( s, response ) { function ajaxConvert( s, response ) {

var conv2, current, conv, tmp,
var conv, conv2, current, tmp,
converters = {}, converters = {},
i = 0, i = 0,
// Work with a copy of dataTypes in case we need to modify it for conversion // Work with a copy of dataTypes in case we need to modify it for conversion
Expand Down
6 changes: 1 addition & 5 deletions src/ajax/xhr.js
View file
Expand Up @@ -101,11 +101,7 @@ if ( xhrSupported ) {


// Listener // Listener
callback = function( _, isAbort ) { callback = function( _, isAbort ) {

var status, responseHeaders, statusText, responses;

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jan 31, 2013

Member

status and statusText separated, why? Unknown by reading the code. That's what I meant by disruptive. Again.

This comment has been minimized.

Sign in to view

Copy link
@scottgonzalez

scottgonzalez Jan 31, 2013

Member

Who is looking at the order of variables that have already been declared and losing time over the chosen algorithm for the existing grouping? Do you honestly look at this and worry about it every time you work on ajax code?

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jan 31, 2013

Member

No, I expect code to make logical sense. So, when I code, I group variables given their meaning/semantics, yes. Sorry you don't care about that, I do. I don't understand why the declarations are separated here while they are clearly related and the names of the variable clearly state they are. By your line of reasonning, who cares about the chosen algorithm for naming variables?

I do care about this kind of things everytime I code (in ajax, jQuery, or any other project) because I want to understand things when I come back 3 months later. This kind of silly out of order declarations are a distraction when I come back to the code to fix something and, so, quite disruptive. I don't wanna have to care about this, which is the whole point I'm making here.

Logical order of declaration is as much a stylistic help than whitespacing stuff properly. Except everybody seems to care about whitespaces but not the actual flow of declarations (or code in general). Which is quite puzzling, if not sad, if you ask me.
var status,
statusText,
responseHeaders,
responses;


// Firefox throws exceptions when accessing properties // Firefox throws exceptions when accessing properties
// of an xhr when a network error occurred // of an xhr when a network error occurred
Expand Down
2 changes: 1 addition & 1 deletion src/attributes.js
View file
Expand Up @@ -290,7 +290,7 @@ jQuery.extend({
}, },


attr: function( elem, name, value ) { attr: function( elem, name, value ) {
var ret, hooks, notxml, var hooks, notxml, ret,
nType = elem.nodeType; nType = elem.nodeType;


// don't get/set attributes on text, comment and attribute nodes // don't get/set attributes on text, comment and attribute nodes
Expand Down
8 changes: 4 additions & 4 deletions src/callbacks.js
View file
Expand Up @@ -46,12 +46,12 @@ jQuery.Callbacks = function( options ) {
memory, memory,
// Flag to know if list was already fired // Flag to know if list was already fired
fired, fired,
// First callback to fire (used internally by add and fireWith)
firingStart,
// Index of currently firing callback (modified by remove if needed)
firingIndex,
// End of the loop when firing // End of the loop when firing
firingLength, firingLength,
// Index of currently firing callback (modified by remove if needed)
firingIndex,
// First callback to fire (used internally by add and fireWith)
firingStart,
// Actual callback list // Actual callback list
list = [], list = [],
// Stack of fire calls for repeatable lists // Stack of fire calls for repeatable lists
Expand Down
10 changes: 5 additions & 5 deletions src/core.js
View file
@@ -1,10 +1,10 @@
var var
// A central reference to the root jQuery(document)
rootjQuery,

// The deferred used on DOM ready // The deferred used on DOM ready
readyList, readyList,


// A central reference to the root jQuery(document)
rootjQuery,

// Support: IE<9 // Support: IE<9
// For `typeof node.method` instead of `node.method !== undefined` // For `typeof node.method` instead of `node.method !== undefined`
core_strundefined = typeof undefined, core_strundefined = typeof undefined,
Expand Down Expand Up @@ -93,7 +93,7 @@ jQuery.fn = jQuery.prototype = {


constructor: jQuery, constructor: jQuery,
init: function( selector, context, rootjQuery ) { init: function( selector, context, rootjQuery ) {
var elem, match; var match, elem;


// HANDLE: $(""), $(null), $(undefined), $(false) // HANDLE: $(""), $(null), $(undefined), $(false)
if ( !selector ) { if ( !selector ) {
Expand Down Expand Up @@ -288,7 +288,7 @@ jQuery.fn = jQuery.prototype = {
jQuery.fn.init.prototype = jQuery.fn; jQuery.fn.init.prototype = jQuery.fn;


jQuery.extend = jQuery.fn.extend = function() { jQuery.extend = jQuery.fn.extend = function() {
var copy, options, src, copyIsArray, name, clone, var src, copyIsArray, copy, name, options, clone,
target = arguments[0] || {}, target = arguments[0] || {},
i = 1, i = 1,
length = arguments.length, length = arguments.length,
Expand Down
6 changes: 3 additions & 3 deletions src/css.js
View file
@@ -1,4 +1,4 @@
var curCSS, getStyles, iframe, var iframe, getStyles, curCSS,
ralpha = /alpha\([^)]*\)/i, ralpha = /alpha\([^)]*\)/i,
ropacity = /opacity\s*=\s*([^)]*)/, ropacity = /opacity\s*=\s*([^)]*)/,
rposition = /^(top|right|bottom|left)$/, rposition = /^(top|right|bottom|left)$/,
Expand Down Expand Up @@ -98,7 +98,7 @@ function showHide( elements, show ) {
jQuery.fn.extend({ jQuery.fn.extend({
css: function( name, value ) { css: function( name, value ) {
return jQuery.access( this, function( elem, name, value ) { return jQuery.access( this, function( elem, name, value ) {
var styles, len, var len, styles,
map = {}, map = {},
i = 0; i = 0;


Expand Down Expand Up @@ -239,7 +239,7 @@ jQuery.extend({
}, },


css: function( elem, name, extra, styles ) { css: function( elem, name, extra, styles ) {
var val, num, hooks, var num, val, hooks,
origName = jQuery.camelCase( name ); origName = jQuery.camelCase( name );


// Make sure that we're working with the right name // Make sure that we're working with the right name
Expand Down
7 changes: 3 additions & 4 deletions src/data.js
View file
@@ -1,7 +1,7 @@
var rbrace = /(?:\{[\s\S]*\}|\[[\s\S]*\])$/, var rbrace = /(?:\{[\s\S]*\}|\[[\s\S]*\])$/,
rmultiDash = /([A-Z])/g; rmultiDash = /([A-Z])/g;


function internalData( elem, name, data, pvt ) { function internalData( elem, name, data, pvt /* Internal Use Only */ ){
if ( !jQuery.acceptData( elem ) ) { if ( !jQuery.acceptData( elem ) ) {
return; return;
} }
Expand Down Expand Up @@ -100,8 +100,7 @@ function internalRemoveData( elem, name, pvt ) {
return; return;
} }


var thisCache, i, l, var i, l, thisCache,

isNode = elem.nodeType, isNode = elem.nodeType,


// See jQuery.data for more information // See jQuery.data for more information
Expand Down Expand Up @@ -216,7 +215,7 @@ jQuery.extend({
_data: function( elem, name, data ) { _data: function( elem, name, data ) {
return internalData( elem, name, data, true ); return internalData( elem, name, data, true );
}, },

_removeData: function( elem, name ) { _removeData: function( elem, name ) {
return internalRemoveData( elem, name, true ); return internalRemoveData( elem, name, true );
}, },
Expand Down
6 changes: 4 additions & 2 deletions src/effects.js
View file
Expand Up @@ -175,7 +175,7 @@ function Animation( elem, properties, options ) {
} }


function propFilter( props, specialEasing ) { function propFilter( props, specialEasing ) {
var index, name, easing, value, hooks; var value, name, index, easing, hooks;


// camelCase, specialEasing and expand cssHook pass // camelCase, specialEasing and expand cssHook pass
for ( index in props ) { for ( index in props ) {
Expand Down Expand Up @@ -243,7 +243,9 @@ jQuery.Animation = jQuery.extend( Animation, {


function defaultPrefilter( elem, props, opts ) { function defaultPrefilter( elem, props, opts ) {
/*jshint validthis:true */ /*jshint validthis:true */
var index, prop, value, length, dataShow, toggle, tween, hooks, oldfire, var prop, index, length,
value, dataShow, toggle,
tween, hooks, oldfire,
anim = this, anim = this,
style = elem.style, style = elem.style,
orig = {}, orig = {},
Expand Down
25 changes: 12 additions & 13 deletions src/event.js
View file
Expand Up @@ -21,10 +21,9 @@ jQuery.event = {
global: {}, global: {},


add: function( elem, types, handler, data, selector ) { add: function( elem, types, handler, data, selector ) {

var tmp, events, t, handleObjIn,
var handleObjIn, tmp, eventHandle, special, eventHandle, handleObj,
t, handleObj, special, handlers, type, namespaces, origType,
events, handlers, type, namespaces, origType,
elemData = jQuery._data( elem ); elemData = jQuery._data( elem );


// Don't attach events to noData or text/comment nodes (but allow plain objects) // Don't attach events to noData or text/comment nodes (but allow plain objects)
Expand Down Expand Up @@ -132,10 +131,10 @@ jQuery.event = {


// Detach an event or set of events from an element // Detach an event or set of events from an element
remove: function( elem, types, handler, selector, mappedTypes ) { remove: function( elem, types, handler, selector, mappedTypes ) {

var j, handleObj, tmp,
var events, handleObj, tmp, origCount, t, events,
j, t, origCount, special, handlers, type,
special, handlers, type, namespaces, origType, namespaces, origType,
elemData = jQuery.hasData( elem ) && jQuery._data( elem ); elemData = jQuery.hasData( elem ) && jQuery._data( elem );


if ( !elemData || !(events = elemData.events) ) { if ( !elemData || !(events = elemData.events) ) {
Expand Down Expand Up @@ -205,8 +204,8 @@ jQuery.event = {
}, },


trigger: function( event, data, elem, onlyHandlers ) { trigger: function( event, data, elem, onlyHandlers ) {

var handle, ontype, cur,
var i, handle, ontype, bubbleType, tmp, special, cur, bubbleType, special, tmp, i,
eventPath = [ elem || document ], eventPath = [ elem || document ],
type = event.type || event, type = event.type || event,
namespaces = event.namespace ? event.namespace.split(".") : []; namespaces = event.namespace ? event.namespace.split(".") : [];
Expand Down Expand Up @@ -343,7 +342,7 @@ jQuery.event = {
// Make a writable jQuery.Event from the native event object // Make a writable jQuery.Event from the native event object
event = jQuery.event.fix( event ); event = jQuery.event.fix( event );


var ret, j, handleObj, matched, i, var i, ret, handleObj, matched, j,
handlerQueue = [], handlerQueue = [],
args = core_slice.call( arguments ), args = core_slice.call( arguments ),
handlers = ( jQuery._data( this, "events" ) || {} )[ event.type ] || [], handlers = ( jQuery._data( this, "events" ) || {} )[ event.type ] || [],
Expand Down Expand Up @@ -398,7 +397,7 @@ jQuery.event = {
}, },


handlers: function( event, handlers ) { handlers: function( event, handlers ) {
var i, matches, sel, handleObj, var sel, handleObj, matches, i,
handlerQueue = [], handlerQueue = [],
delegateCount = handlers.delegateCount, delegateCount = handlers.delegateCount,
cur = event.target; cur = event.target;
Expand Down Expand Up @@ -511,7 +510,7 @@ jQuery.event = {
mouseHooks: { mouseHooks: {
props: "button buttons clientX clientY fromElement offsetX offsetY pageX pageY screenX screenY toElement".split(" "), props: "button buttons clientX clientY fromElement offsetX offsetY pageX pageY screenX screenY toElement".split(" "),
filter: function( event, original ) { filter: function( event, original ) {
var eventDoc, doc, body, var body, eventDoc, doc,
button = original.button, button = original.button,
fromElement = original.fromElement; fromElement = original.fromElement;


Expand Down
12 changes: 7 additions & 5 deletions src/manipulation.js
View file
Expand Up @@ -285,7 +285,8 @@ jQuery.fn.extend({
// Flatten any nested arrays // Flatten any nested arrays
args = core_concat.apply( [], args ); args = core_concat.apply( [], args );


var scripts, node, doc, fragment, hasScripts, first, var first, node, hasScripts,
scripts, doc, fragment,
i = 0, i = 0,
l = this.length, l = this.length,
set = this, set = this,
Expand Down Expand Up @@ -436,7 +437,7 @@ function cloneCopyEvent( src, dest ) {
} }


function fixCloneNodeIssues( src, dest ) { function fixCloneNodeIssues( src, dest ) {
var nodeName, data, e; var nodeName, e, data;


// We do not need to do anything for non-Elements // We do not need to do anything for non-Elements
if ( dest.nodeType !== 1 ) { if ( dest.nodeType !== 1 ) {
Expand Down Expand Up @@ -559,7 +560,7 @@ function fixDefaultChecked( elem ) {


jQuery.extend({ jQuery.extend({
clone: function( elem, dataAndEvents, deepDataAndEvents ) { clone: function( elem, dataAndEvents, deepDataAndEvents ) {
var clone, node, srcElements, i, destElements, var destElements, node, clone, i, srcElements,
inPage = jQuery.contains( elem.ownerDocument, elem ); inPage = jQuery.contains( elem.ownerDocument, elem );


if ( jQuery.support.html5Clone || jQuery.isXMLDoc(elem) || !rnoshimcache.test( "<" + elem.nodeName + ">" ) ) { if ( jQuery.support.html5Clone || jQuery.isXMLDoc(elem) || !rnoshimcache.test( "<" + elem.nodeName + ">" ) ) {
Expand Down Expand Up @@ -614,7 +615,8 @@ jQuery.extend({
}, },


buildFragment: function( elems, context, scripts, selection ) { buildFragment: function( elems, context, scripts, selection ) {
var contains, elem, j, tmp, tag, wrap, tbody, var j, elem, contains,
tmp, tag, tbody, wrap,
l = elems.length, l = elems.length,


// Ensure a safe fragment // Ensure a safe fragment
Expand Down Expand Up @@ -740,7 +742,7 @@ jQuery.extend({
}, },


cleanData: function( elems, /* internal */ acceptData ) { cleanData: function( elems, /* internal */ acceptData ) {
var elem, id, type, data, var elem, type, id, data,
i = 0, i = 0,
internalKey = jQuery.expando, internalKey = jQuery.expando,
cache = jQuery.cache, cache = jQuery.cache,
Expand Down
4 changes: 3 additions & 1 deletion src/support.js
View file
@@ -1,6 +1,8 @@
jQuery.support = (function() { jQuery.support = (function() {


var support, all, a, select, opt, input, fragment, eventName, isSupported, i, var support, all, a,
input, select, fragment,
opt, eventName, isSupported, i,
div = document.createElement("div"); div = document.createElement("div");


// Setup // Setup
Expand Down

0 comments on commit d79bf35

Please sign in to comment.