Eliminate use of isPlainObject and deprecate the method

[david-mark]

Description

The isPlainObject function is complicated and unnecessary (certainly internally). I count five cases where it is used in the core. Here is one example:

jQuery.each( [ "get", "post" ], function( i, method ) {
	jQuery[ method ] = function( url, data, callback, type ) {

		// Shift arguments if data argument was omitted
		if ( jQuery.isFunction( data ) ) {
			type = type || callback;
			callback = data;
			data = undefined;
		}

		// The url can be an options object (which then must have .url)
		return jQuery.ajax( jQuery.extend( {
			url: url,
			type: method,
			dataType: type,
			data: data,
			success: callback
		}, jQuery.isPlainObject( url ) && url ) );
	};
} );

This sort of faux "overloading" is generally ill-advised in ECMAScript. The above code should make that clear as it is relatively inefficient and hard to follow. We won't clean it up entirely here as can't break compatibility (and one thing at a time).

What do we have in this case? A string or an Object object. If it is an object, then it is "mixed in" to the object created to house the various options. Obviously this is a strange interface as we could pass method, callback and type twice (once as the named arguments and again in the url object).

How about this instead?

jQuery.each( [ "get", "post" ], function( i, method ) {
    jQuery[ method ] = function( url, data, callback, type ) {

        // Shift arguments if data argument was omitted
        
        if ( jQuery.isFunction( data ) ) {
            type = type || callback;
            callback = data;
            data = undefined;
        }
        
        var options = {
            type: method,
            dataType: type,
            data: data,
            success: callback
        };
        
        // If url is a string...
        
        if (typeof url == 'string') {
            options.url = url;
        } else {
            
            // If not a string, the only other *allowed* possibility is an Object object
            // Calls with anything *other* than a string or an Object object for url
            // will have undefined behavior.
            // The object must have a "url" property, otherwise behavior is also undefined
            
            jQuery.extend(options, url);
        }
        
        return jQuery.ajax( options );
    };
} );

And let's get rid of that isFunction call as well as that method should get the same treatment in another issue. Just foreshadowing here:

jQuery.each( [ "get", "post" ], function( i, method ) {
    jQuery[ method ] = function( url, data, callback, type ) {

        // Shift arguments if data argument was omitted
        
        if ( typeof data == 'function' ) {
            type = type || callback;
            callback = data;
            data = undefined;
        }
        
        var options = {
            type: method,
            dataType: type,
            data: data,
            success: callback
        };
        
        // If url is a string...
        
        if (typeof url == 'string') {
            options.url = url;
        } else {
            
            // If not a string, the only other *allowed* possibility is an Object object
            // Calls with anything *other* than a string or an Object object for url
            // will have undefined behavior.
            // The object must have a "url" property, otherwise the behavior is also undefined
            
            jQuery.extend(options, url);
        }
        
        return jQuery.ajax( options );
    };
} );

Better, right? Faster and much easier to follow. There's enough going on in that function to start without adding calls to other odd functions.

It's important to understand why isFunction isn't needed here (or anywhere most likely). It's a callback, which must be a Function object. Not any old callable object (e.g. host objects), but an object constructed by Function. All such Function objects have the same typeof result in all browsers: 'function'.

That's one down and four to go. Not going to bother with the rest until there is some indication of agreement that this strategy is sound. It will be much the same deal for the rest of the calls to this method and then it can be deprecated (shouldn't encourage developers to use such functions either). In the bigger picture, there is a lot more in the core that can be made easier to follow through similar restructuring.

Link to test case

No test case. Behavior is to remain the same, so existing test cases are appropriate.

[gibson042]

Ignoring the second part (which is a logically separate issue), I'm fine with switching this type check from object to string, provided it doesn't increase the gzipped size of jquery.min.js.

[david-mark]

Ignoring the second part (which is a logically separate issue), I'm fine with switching this type check from object to string, provided it doesn't increase the gzipped size of jquery.min.js.

Thanks, but we're not yet done here.

Here's the thing "Brain": that was a sample of 5 required changes before isPlainObject is deprecated and eventually deleted, which will easily make up for any additions incurred.

The idea of checking every commit to see whether it added a few bytes to the file size is just wrong. It's premature... something. Call it premature counting of bytes. It's like checking the gas level to the ounce every time you hit a stop light.

Leave it until a trend develops, which will most likely be a downward one for my fork (there's plenty of fat to trim in this project). But what if over time it increased it 1K? Or 2K? There are other factors to weigh against size increases (e.g. code clarity, performance).

Anyway, on to round two...

[david-mark]

Second one is easy as well. From the param method:

        var prefix,
		s = [],
		add = function( key, valueOrFunction ) {

			// If value is a function, invoke it and use its return value
			var value = jQuery.isFunction( valueOrFunction ) ?
				valueOrFunction() :
				valueOrFunction;

			s[ s.length ] = encodeURIComponent( key ) + "=" +
				encodeURIComponent( value == null ? "" : value );
		};
        // If an array was passed in, assume that it is an array of form elements.
	if ( jQuery.isArray( a ) || ( a.jquery && !jQuery.isPlainObject( a ) ) ) {

		// Serialize the form elements
		jQuery.each( a, function() {
			add( this.name, this.value );
		} );

	} else {

		// If traditional, encode the "old" way (the way 1.3.2 or older
		// did it), otherwise encode params recursively.
		for ( prefix in a ) {
			buildParams( prefix, a[ prefix ], traditional, add );
		}
	}

        // Return the resulting serialization
	return s.join( "&" );

Clearly there are three possibilities for a; though the comments only mention one, we can infer the other two:

1. Array object
2. Object object constructed with jQuery
3. Object object not constructed with jQuery

#2 objects have jQuery on their prototype chain and #3 objects do not.

Bad variable name in a, but will leave that for the moment.

This line checks if a is either an Array or:

if ( jQuery.isArray( a ) || ( a.jquery && !jQuery.isPlainObject( a ) ) ) {

...Something that is an Object not constructed by the base Object object constructor (call it a "fancy " object?) and that has a "truthy" jquery property. That's a fairly poor duck typing inference as what we clearly require for the next line is an array-like object (that is apparently assumed to contain references to form controls).

How about this direct inference (and we'll get to isArray in a future issue):

if ( jQuery.isArray( a ) || a instanceof jQuery ) {

Fair enough? We still make the assumption about the contents of the jQuery array-like object, but should simply be documented that passing any other variation of a jQuery object (e.g. containing a document reference) will lead to undefined behavior.

I know it will come up, so will head it off now. What about frames? The short answer is: what about them? If we have a frameset (unlikely) or an IFrame in our document with another jQuery (would be required as jQuery only deals with a single window at a time), we would simply use the jQuery.param in the other frame.

Wouldn't make any sense to pass a jQuery object constructed in one frame to jQuery.param in another frame. In any event, that could be documented (though I wouldn't bother). The function would still work, provided that the frame's Object.prototype object has not been ill-advisedly augmented.

In other words, it would take a perfect storm of incompetence on the part of the caller to break this. On the other hand, it's trivially easy to break the original indirect inference.

Finally, let's dump the isFunction reference while we are at it as it is more straightforward to eliminate than isArray:

        var prefix,
		s = [],
		add = function( key, valueOrFunction ) {

			// If value is a function, invoke it and use its return value
			var value = typeof valueOrFunction == 'function' ?
				valueOrFunction() :
				valueOrFunction;

			s[ s.length ] = encodeURIComponent( key ) + "=" +
				encodeURIComponent( value == null ? "" : value );
		};
        // If an array was passed in, assume that it is an array (or jQuery object) of form elements.
	if ( jQuery.isArray( a ) || a instanceof jQuery ) {

		// Serialize the form elements
		jQuery.each( a, function() {
			add( this.name, this.value );
		} );

	} else {

		// If traditional, encode the "old" way (the way 1.3.2 or older
		// did it), otherwise encode params recursively.
		for ( prefix in a ) {
			buildParams( prefix, a[ prefix ], traditional, add );
		}
	}

        // Return the resulting serialization
	return s.join( "&" );

That leaves three to go: one in init, which will require a little more inspection and thought than the first two (due to "overloading" involving host objects). The other two are in extend, which appears to be a variation on the old mixin function.

What I expect is that the two in init will be fixed first and that will leave a single function calling isPlainObject. So no matter how insidious that function (and appears to be truly nightmarish code), the appropriate bits of isPlainObject (if any) can simply be folded into it. Then we can deprecate and eventually drop isPlainObject. And yes, this may increase the code size temporarily; sometimes that's just the cost of progress (and why it is important to get a design right the first time). Ultimately, we will have a net decrease once isPlainObject is removed. Not to mention that isFunction is creeping towards extinction along with it (though far more slowly as there are dozens of references to it).

Will look at the next one when I have time and after confirmation that we are all on the same page with these changes.

[gibson042]

the appropriate bits of isPlainObject (if any) can simply be folded into it

isPlainObject doesn't have any boilerplate, so this seems to be at odds with your claim of a net size reduction. But I'm willing to be proven wrong.

[gibson042]

Also, a instanceof jQuery is not compatible with separate instances of jQuery as and is unacceptable. .jquery is our canonical method of detecting jQuery collections.

[david-mark]

isPlainObject doesn't have any boilerplate, so this seems to be at odds with your claim of a net size reduction. But I'm willing to be proven wrong.

I feel quite sure that I won't require every line in that function to eliminate the call from extend. On the other hand, what if I am wrong? Still leaves the performance and clarity benefits, the latter of which makes our code easier to debug and maintain. Seriously doubt that any increase in the size due to these proposed changes will overshadow these benefits. But we'll see.

Let me know on round 2...

[david-mark]

Also, a instanceof jQuery is not compatible with separate instances of jQuery as is
unacceptable. .jquery is our canonical method of detecting jQuery collections.

Did you read about the dueling scenarios? The existing one is trivial to break (however canonical you consider the test), the other would require such a bizarre turn of events (including more than just using separate instances) that it's not even worth considering. So should be clear which makes more sense.

Furthermore, are we not allowed to pass non-array-like "fancy" objects with a "truthy" jquery property? All seems very complicated to document and all in the name of allowing one instance of jQuery to call another's param method? Makes my head hurt just thinking about it.

Regardless, we can make a more direct inference to determine if it is an array-like object containing form control elements (or simply an array-like object as we are clearly assuming what the contents will be). Or perhaps validate that it is a jQuery object with something like:

a.jquery && !Object.hasOwnProperty.call(a, 'jquery')

Note: Only calling hasOwnProperty that way to cover the odd chance that a lacks a prototype as this is a case where a such an object would actually make sense.

Think it over and I'll check in tomorrow. Best of luck!

PS. One other hint: if Object.prototype has been augmented, jQuery will break in many places due to "unsafe" for-in loops.

[gibson042]

And if it is canonical, then could just check if it is any object with that property. Definitely will not require isPlainObject here.

jQuery.param makes the isPlainObject check explicit to allow jQuery.param({jquery: value}).

[david-mark]

jQuery.param makes the isPlainObject check explicit to allow jQuery.param({jquery: value}).

See my edited response above. Sorry, but I didn't notice the new reply in the interim.

[david-mark]

I found this line from init interesting in light of the previous discussions of canon:

context = context instanceof jQuery ? context[ 0 ] : context;

Clearly the canon is not consistent throughout.

[david-mark]

Three to go...

Here is the snippet from init. Mistakenly said earlier that init had two calls. It's just the one with the remaining two in extend.

// HANDLE: $(html) -> $(array)
if ( match[ 1 ] ) {
    context = context instanceof jQuery ? context[ 0 ] : context;

    // Option to run scripts is true for back-compat
    // Intentionally let the error be thrown if parseHTML is not present
    jQuery.merge( this, jQuery.parseHTML(
        match[ 1 ],
        context && context.nodeType ? context.ownerDocument || context : document,
        true
    ) );

    // HANDLE: $(html, props)
    if ( rsingleTag.test( match[ 1 ] ) && jQuery.isPlainObject( context ) ) {
        for ( match in context ) {
            // Properties of context are called as methods if possible
            if ( jQuery.isFunction( this[ match ] ) ) {
                this[ match ]( context[ match ] );

                // ...and otherwise set as attributes
            } else {
                this.attr( match, context[ match ] );
            }
        }
    }

    return this;

// HANDLE: $(#id)
} else {

And the line in play is:

if ( rsingleTag.test( match[ 1 ] ) && jQuery.isPlainObject( context ) ) {

This is the case where match[ 1 ] is the HTML (string) representing a single "tag" (element) and context is in fact an Object object with properties representing (ahem) attributes or whatever attr does with names and values these days. Believe it is (mostly) DOM properties in HTML and attributes in XML, but that's another, long and complicated story (and irrelevant to the problem at hand).

So what are we attempting to discern here? A clue is in a line above:

context && context.nodeType ? context.ownerDocument || context

Apparently context can be a document, element or... something else. But what is it allowed to be in the case? Appears the possibilities beyond a host object include undefined or an Object object. Anything else would lead to undefined behavior. Well, will throw null in there too as jQuery lumps those two together when type checking arguments. So let's say:

1. Something "falsey" (e.g. undefined or null values)
2. Element or document host object
3. Object object representing properties or attributes to set on the single element

Now we have an idea of the sort of test we need and an example in this same function that indicates what seems to be an easy solution.

We can eliminate #1 by simply asserting that context is "truthy". We also know that nodeType is not an appropriate property (or attribute) to set on an element. So, this would seem like the simplest and most straightforward solution:

if ( rsingleTag.test( match[ 1 ] ) && context && !context.nodeType) {

And it is also consistent with the previous duck typing in this function. Furthermore, it allows "fancy" objects (i.e. constructed with something other than Object) to house the name/value pairs passed to attr. Why not? Doesn't seem like a common use case, but can't imagine any reason why such objects should be disallowed.

But never like to see boolean type conversion on host object properties as they have been known to throw exceptions in the past (and history is our only guide for such host object reactions), so this is more appropriate:

if ( rsingleTag.test( match[ 1 ] ) && context && typeof context.nodeType != 'number' ) {

And once again, let's knock out the unneeded isFunction call while at it:

// HANDLE: $(html) -> $(array)
if ( match[ 1 ] ) {
    context = context instanceof jQuery ? context[ 0 ] : context;

    // Option to run scripts is true for back-compat
    // Intentionally let the error be thrown if parseHTML is not present
    jQuery.merge( this, jQuery.parseHTML(
        match[ 1 ],
        context && typeof context.nodeType == 'number' ? context.ownerDocument || context : document,
        true
    ) );

    // HANDLE: $(html, props)
    if ( rsingleTag.test( match[ 1 ] ) && context && typeof context.nodeType != 'number' ) {
        for ( match in context ) {
            // Properties of context are called as methods if possible
            if ( typeof this[ match ] == 'function' ) {
                this[ match ]( context[ match ] );

                // ...and otherwise set properties or attributes (depending on DOM type)
            } else {
                this.attr( match, context[ match ] );
            }
        }
    }

    return this;

// HANDLE: $(#id)
} else {

Updated the comment about attr as well. Correct me if I'm wrong about what that does. I know it once set properties or attributes depending on DOM type and then it didn't and then it did again, right? Somewhere around 1.6 when it was realized that attr alone was impossible to make work and prop was added as its companion?

That leaves just extend calling isPlainObject; so we can simply identify which bits (if any) are still required, add them to extend and deprecate isPlainObject. Will wait to make sure there are no objections to these suggestions before diving into that one. Certainly extend will get the biggest performance boost, particularly when extending "deep".

[dmethvin]

Let's take a step back.

What exactly are these changes intended to accomplish? Generally we revisit code when users report bugs or performance issues. Refactoring code that doesn't have problems can introduce compatibility problems and requires the time and attention of multiple people (you and team members). The best time to do this type of refactoring is when that region of code has other reported issues and the refactoring helps to resolve the issue (and in the process, makes the code better).

We have a few dozen issues that would be a great starting point for changing code that would also fix real problems that users have reported.

[david-mark]

Let's take a step back.

AFAIK, we have yet to take a step forward. I'm proposing several steps forward and we'll see where that takes us (or not).

What exactly are these changes intended to accomplish?

As I've explained: clarity, performance and eliminating the crutch of isPlainObject, which will soon be followed by other is* functions as they just encourage poor design (as we've seen). I'm untangling the code as best I can while hemmed in to design decisions that date back a decade and will likely never be changed; regardless, am making good progress. Long term, expect to reduce the amount of code as well (and code size seems to be a major focus here).

Generally we revisit code when users report bugs or performance issues.

I'd say that's led to the state that the code is in. How exactly do you define a "performance issue"? I'm looking at the code and seeing all sorts of unneeded function calls, which are expensive. Furthermore, the untangling process has identified inconsistencies (e.g. use of instanceof in one place and duck typing in another to try to sniff out a jQuery object), inefficiencies, redundancies and code that is harder to follow and less clear about what it is doing than it should be (which makes it harder to document).

Refactoring code that doesn't have problems can introduce compatibility problems and
requires the time and attention of multiple people (you and team members).

Not sure how you define "problems", but it goes without saying that refactoring code can break compatibility and requires time and attention (which I'm providing).

I consider obscure and inconsistent code to be a problem to document and harder to maintain than clear and consistent code. Every time you go to patch a bug, the chance of creating more bugs is tied to the clarity and consistency of the code.

I also know from experience (as well as from reading the code) that jQuery is relatively slow in virtually everything it does (remember the JSPerf comparisons I posted comparing to My Library?) No coincidence that I know what to do to improve the performance, just as I did for Dojo several years back. Some of them balked too and I'd urge you not to follow in their footsteps as they certainly don't lead forward.

We have a few dozen issues that would be a great starting point for changing code that would also fix real problems that users have reported.

And good luck with those bug fixes. That's not what I'm doing here at the moment. Feel free to use or not use whatever you want; that's why I'm explaining everything in detail as I go. If you really feel you have a pat hand because nobody is complaining, I'd advise you to reevaluate that position. ;)

Recall, that no users reported any real problems with UA sniffing. On the other hand, there was never any shortage of issues related to the attr, but the specific causes were never pinpointed. Most complaints were of the "it doesn't work" variety and the code was clearly hard to follow and under-documented as to what it was supposed to be doing. The myriad unit tests (and testers) weren't helping to identify these problems either as the attr code simply made no sense. Sure it seemed to "work" per the prescribed tests, but after five years or so it became clear that the tests were simply insufficient to diagnose problems with the design. Much of the code did meet the vaguely documented expectations by coincidence, but it was so hard to follow and expectations were so murky that deviations were simply written off as "edge cases".

Another such coincidence (related to SELECT elements) was pointed out years ago and is still in the code today. Sure the unit tests and users have yet to complain about that code either, though it may well have caused issues that were also dismissed as "edge cases", sun spots, problems with the phase of the moon, etc. I'll get to that one, or perhaps somebody else can have a go at it while I deal with the issue at hand. That problem and its solution have been discussed in public numerous times over the years. Ironically, the solution was dismissed because it may have added an extra line or two (sound familiar?) and Resig couldn't see any evidence that the extant code didn't "work" (i.e. no complaints).

Problems in philosophy and design can't be patched (or fit neatly into tickets). Piling up patches without a long-term vision is exactly how code ends up convoluted and inconsistent (and that much harder to patch without creating new issues).

Don't expect users to evaluate the clarity of the code or the performance either, though they may compare the latter to what they can get from similar libraries. The only way to stay ahead of the curve is to be proactive.

But I digress; at the moment I'm dealing (mostly) with the isPlainObject crutch and unable to deal with the design issues that created it. Happily, it appears this one will go away entirely without changing the API all. Similar methods may not go as quietly, but we'll see. The idea is that future methods will not have such crutches available and so will require designs more appropriate for ECMAScript and browser scripting.

Stay tuned...

[david-mark]

After inspecting jQuery.extend, it's clear to me why the logic is so convoluted. It's an iterative "mixin" mashed up with a recursive "cloneDeep" (two classic JS functions).

I will spin off the recursive bit to make it simpler to follow. And yes, I am sure that I will break this issue into at least two as well. I don't care to mess with the method at all in its present state. Will be much simpler to see what I'm doing after splitting it.

Once I do that, I'll wrap up the isPlainObject abatement and split up this issue. This one will likely be renamed to something like "Reduce dependency on isPlainObject" and the second will be the final nail that also restructures extend. If we really need a third to replace the isFunction calls then fine, but seems like those should be able to go in the same commits; perhaps we can just change the issue name to reflect that too.

[timmywil]

I see your point about the old nature of attr and its inherent problems that didn't necessarily show up in the issues or tests, but it was still bug reports that led us to refactoring that code (which, by the way, John had wanted to do for a long time). However, I doubt we'll be removing jQuery.isPlainObject because it's a documented method without an obvious native equivalent to which we can point. So, I don't want to go ripping up the guts of this method without any bug reports to motivate us.

Besides, you may be right about the needs here, but we've discussed it and given that we have limited time to work on things–we're all volunteers after all–we've decided we'd like to focus on more important issues based on practical use cases. isPlainObject may not be perfect, but we won't be considering changes for the moment.

Thank you for your contributions. We'd love it if you'd like to pick an existing issue to help with.

[david-mark]

I see your point about the old nature of attr and its inherent problems that didn't necessarily show up in the issues or tests, but it was still bug reports that led us to refactoring that code (which, by the way, John had wanted to do for a long time)

Didn't you write a blog post about five years after I first mentioned the attr design problem in a Usenet review? I think the quote was along the lines of: "I always suspected something was wrong with attr but..." :)

It was a design problem and required adding a companion prop method as lumping attributes and properties into one odd method was never going to work. Suppose the point is that if you'd have listened to me back then... Also that users are not the best source of information on such odd failures as they tend not to provide enough details about why their code "doesn't work".

We have history repeating itself here. There may well have been reports of failures related to some of the issues I've kicked up, but they could have fallen through the cracks here or on StackOverflow (or on Usenet in years past). Why wait to get a coherent bug report from users when we can see the bugs right now?

And if John had wanted to fix attr for a long time, then all he had to do was to deprecate the method and replace it with two new ones (as had been suggested repeatedly on Usenet). That wasn't the solution arrived at five years later; as a result, the project had all sorts of new issues to deal with and was rightly criticized as a result (from all quarters).

Have personally dealt with projects that were frozen in time supporting IE 7 or 8 at best, simply because they were using an old version of jQuery with the old attr and it wouldn't work in IE 9 or 10 or whatever they aspired to upgrade to. Have also seen projects that couldn't upgrade jQuery because the new attr broke their old code. Nobody involved knew what that black box did or even that it was the cause of their problems, so nobody reported anything to you. My fix or advice was always to remove jQuery from the equation as nobody ever needed it to get or set properties in the first place. But I digress.

However, I doubt we'll be removing jQuery.isPlainObject because it's a documented method without an obvious native equivalent to which we can point.

The point is that it should be deprecated (not removed any time soon) as there is no reason for us or the users to rely on such a crutch. And shouldn't increase the code size much (if at all) to address this concern (as well as the several related concerns uncovered in dealing with it here). If you think about it, isPlainObject has nothing to do with the jQuery API, which would be one unneeded function simpler if it were removed in future. It's simply an extraneous function that nobody should ever want or need. Granted, it's there and we are using it internally (my main issue) and surely users are as well (to their detriment).

So, I don't want to go ripping up the guts of this method without any bug reports to motivate us.

I never suggested ripping up the guts of it at all, simply deprecating it. It's the other methods that require some ripping up due to inconsistencies, redundancies and inefficiencies uncovered during the investigation of this issue. Though not impossible, it's unlikely you will get motivation to fix these issues due to bug reports. They are simply rare cases (or cause only performance issues). Motivation to fix such things needs to come from within.

Your best motivation should be to keep the issues from multiplying. The easier the code is to follow, the easier it is to debug. Misunderstandings can lead to the creation of new bugs when dealing with reported issues. Should be particularly concerned about bugs that are flying under the radar of the unit tests.

But also realize that this is not just about bugs, but about unclear code and silent failures. A call to isPlainObject implies that the passed value could be anything. In one function there are two cases where it could be used, but it is only used in one. However, there's no good reason to use it in either case as the value in question has only a handful of allowed possibilities. Again, hard to follow, inconsistent and therefore difficult to document for the users and more complicated to test than it should be. Will also find redundancy in that example as both cases test for a "truthy" value before moving on the rest of the test. Certainly one of the first checks isPlainObject does is for "falsey" values.

Besides, you may be right about the needs here, but we've discussed it and given that we have limited time to work on things–we're all volunteers after all–we've decided we'd like to focus on more important issues based on practical use cases.

I don't understand why you need to close the issue to allocate your focus. Contend it should be reopened, at least until I finish with it. Then those who have time can chime in.

isPlainObject may not be perfect, but we won't be considering changes for the moment.

I never suggested changing one line in isPlainObject. All of my suggest changes to eliminate its use internally are elsewhere.

Thank you for your contributions. We'd love it if you'd like to pick an existing issue to help with.

Thanks and that could happen, but as I am also a volunteer, I will work on what I am most motivated to do at any given time. Right now, I am motivated to wrap this issue up and paste the code into my fork. Whether it ever gets merged is irrelevant to me.

May also add additional issues springing from this one (e.g. breaking up extend). I strongly suggest that everybody re-read these discussions very carefully as several mistakes have been uncovered and I can see no sense in waiting around for users to report them, no matter how obscure the cases may be. The more obscure, the less likely users are to report them, yet virtually everything jQuery does relies on these low-level functions (e.g. init). Furthermore, the suggested fixes are very brief and simple to review, provided we all understand exactly what this code does (and how it differs from what it is documented to do).

Best of luck!

[david-mark]

Re-did extend and without splitting it in two. Turns out that wouldn't have made much difference.

#3444

As expected, didn't need near the amount of code in isPlainObject for extend. The needed bits (slightly rewritten for clarity and speed) are now "private" to extend. The jQuery.isPlainObject method is now deprecated.

isPlainObject: function( obj ) {

	/* eslint-disable no-undef */
	// This function is conditionally created, depending on whether console.warn exists
	// NOTE: This is "scaffolding" code that can be removed in production builds
        if ( deprecated ) {
	       deprecrated( "isPlainObject" );
        }

The deprecated function, which will be used in other soon to follow methods:

/* eslint-disable no-unused-vars */
// Conditionally created function is used inside functions when it exists
var deprecated = null;

if ( typeof window.console != "undefined" && typeof window.console.warn != "undefined" ) {
	deprecated = jQuery.deprecated = function( feature, alternative ) {
		alternative = alternative || "Please find another way.";
		window.console.warn( feature + " is deprecated! " + alternative );
	};
}

Though there used to be a similar function, but could only find code like this in Deferred exception handling. Should likely restructure so that all of the code can share a common warn function.

And that's that. As mentioned in the new extend rehab issue, the lessons learned during the investigation and adjustment of the affected code is indicative of the sort of work that is required throughout this project (if it is to get off the treadmill of nagging bug fixes). See this as the only way to regain and maintain any semblance of relevancy.

There are other problems unrelated to this theme (e.g. multi-browser builds), but it would be pointless to get into them until these basic issues are addressed.

Strongly advise reopening this ticket. Even if we buy into none of the above, there are clear mistakes and inconsistencies that have been uncovered in previous comments. Read it carefully from the top and they should be apparent.