Skip to content

Commit 0ea1015

Browse files
mgolmgol
authored
Core: Make migrateWarnProp/etc. not return props for disabled patches
Changes: 1. Make `migrateWarnProp` / `migratePatchAndWarnFunc` and other similar functions not return the APIs when patches are disabled. Previously, `migrateWarnProp` would only stop printing warnings but the API would still be patched, while `migratePatchAndWarnFunc` would return a noop function instead of `undefined`. 2. Don't warn just on access of `jQuery.fn.push`, etc., but when it's called. 3. Rename some internal methods. Closes gh-571
1 parent 94ab4c5 commit 0ea1015

File tree

3 files changed

+86
-32
lines changed

3 files changed

+86
-32
lines changed

‎src/jquery/core.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { migrateWarnProp, migratePatchAndWarnFunc, migratePatchAndInfoFunc } from "../main.js";
1+
import { migratePatchAndWarnFunc, migratePatchAndInfoFunc } from "../main.js";
22
import "../disablePatches.js";
33

44
var arr = [],
@@ -92,9 +92,9 @@ migratePatchAndWarnFunc( jQuery, "isWindow",
9292
migratePatchAndInfoFunc( jQuery, "proxy", jQuery.proxy,
9393
"proxy", "jQuery.proxy() is deprecated" );
9494

95-
migrateWarnProp( jQuery.fn, "push", push, "push",
95+
migratePatchAndWarnFunc( jQuery.fn, "push", push, "push",
9696
"jQuery.fn.push() is removed; use .add() or convert to an array" );
97-
migrateWarnProp( jQuery.fn, "sort", sort, "sort",
97+
migratePatchAndWarnFunc( jQuery.fn, "sort", sort, "sort",
9898
"jQuery.fn.sort() is removed; convert to an array before sorting" );
99-
migrateWarnProp( jQuery.fn, "splice", splice, "splice",
99+
migratePatchAndWarnFunc( jQuery.fn, "splice", splice, "splice",
100100
"jQuery.fn.splice() is removed; use .slice() or .not() with .eq()" );

‎src/main.js

+50-23
Original file line numberDiff line numberDiff line change
@@ -70,38 +70,71 @@ export function migrateInfo( code, msg ) {
7070
migrateMessageInternal( code, msg, "info" );
7171
}
7272

73-
function migrateMessagePropInternal(
73+
function migratePatchPropInternal(
7474
obj, prop, value, code, msg, migrateMessageFn
7575
) {
76+
var orig = obj[ prop ];
7677
Object.defineProperty( obj, prop, {
7778
configurable: true,
7879
enumerable: true,
80+
7981
get: function() {
80-
migrateMessageFn( code, msg );
81-
return value;
82+
if ( jQuery.migrateIsPatchEnabled( code ) ) {
83+
84+
// If `msg` not provided, do not message; more sophisticated
85+
// messaging logic is most likely embedded in `value`.
86+
if ( msg ) {
87+
migrateMessageFn( code, msg );
88+
}
89+
90+
return value;
91+
}
92+
93+
return orig;
8294
},
95+
8396
set: function( newValue ) {
84-
migrateMessageFn( code, msg );
85-
value = newValue;
97+
if ( jQuery.migrateIsPatchEnabled( code ) ) {
98+
99+
// See the comment in the getter.
100+
if ( msg ) {
101+
migrateMessageFn( code, msg );
102+
}
103+
}
104+
105+
// If a new value was set, apply it regardless if
106+
// the patch is later disabled or not.
107+
orig = value = newValue;
86108
}
87109
} );
88110
}
89111

90112
export function migrateWarnProp( obj, prop, value, code, msg ) {
91-
migrateMessagePropInternal( obj, prop, value, code, msg, migrateWarn );
113+
if ( !msg ) {
114+
throw new Error( "No warning message provided" );
115+
}
116+
migratePatchPropInternal( obj, prop, value, code, msg, migrateWarn );
92117
}
93118

94119
export function migrateInfoProp( obj, prop, value, code, msg ) {
95-
migrateMessagePropInternal( obj, prop, value, code, msg, migrateInfo );
120+
if ( !msg ) {
121+
throw new Error( "No warning message provided" );
122+
}
123+
migratePatchPropInternal( obj, prop, value, code, msg, migrateInfo );
124+
}
125+
126+
export function migratePatchProp( obj, prop, value, code ) {
127+
migratePatchPropInternal( obj, prop, value, code );
96128
}
97129

98-
function migrateMessageFuncInternal(
130+
// The value of the "Func" APIs is that for method we want to allow
131+
// checking for the method existence without triggering a warning.
132+
// For other deprecated properties, we do need to warn on access.
133+
function migratePatchFuncInternal(
99134
obj, prop, newFunc, code, msg, migrateMessageFn
100135
) {
101-
var finalFunc,
102-
origFunc = obj[ prop ];
103136

104-
obj[ prop ] = function() {
137+
function wrappedNewFunc() {
105138

106139
// If `msg` not provided, do not warn; more sophisticated warnings
107140
// logic is most likely embedded in `newFunc`, in that case here
@@ -111,34 +144,28 @@ function migrateMessageFuncInternal(
111144
migrateMessageFn( code, msg );
112145
}
113146

114-
// Since patches can be disabled & enabled dynamically, we
115-
// need to decide which implementation to run on each invocation.
116-
finalFunc = jQuery.migrateIsPatchEnabled( code ) ?
117-
newFunc :
118-
119-
// The function may not have existed originally so we need a fallback.
120-
( origFunc || jQuery.noop );
147+
return newFunc.apply( this, arguments );
148+
}
121149

122-
return finalFunc.apply( this, arguments );
123-
};
150+
migratePatchPropInternal( obj, prop, wrappedNewFunc, code );
124151
}
125152

126153
export function migratePatchAndWarnFunc( obj, prop, newFunc, code, msg ) {
127154
if ( !msg ) {
128155
throw new Error( "No warning message provided" );
129156
}
130-
return migrateMessageFuncInternal( obj, prop, newFunc, code, msg, migrateWarn );
157+
return migratePatchFuncInternal( obj, prop, newFunc, code, msg, migrateWarn );
131158
}
132159

133160
export function migratePatchAndInfoFunc( obj, prop, newFunc, code, msg ) {
134161
if ( !msg ) {
135162
throw new Error( "No info message provided" );
136163
}
137-
return migrateMessageFuncInternal( obj, prop, newFunc, code, msg, migrateInfo );
164+
return migratePatchFuncInternal( obj, prop, newFunc, code, msg, migrateInfo );
138165
}
139166

140167
export function migratePatchFunc( obj, prop, newFunc, code ) {
141-
return migrateMessageFuncInternal( obj, prop, newFunc, code );
168+
return migratePatchFuncInternal( obj, prop, newFunc, code );
142169
}
143170

144171
if ( window.document.compatMode === "BackCompat" ) {

‎test/unit/migrate.js

+32-5
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ QUnit.test( "jQuery.migrateDeduplicateMessages", function( assert ) {
4646
} );
4747

4848
QUnit.test( "disabling/enabling patches", function( assert ) {
49-
assert.expect( 14 );
49+
assert.expect( 27 );
5050

5151
var elem = jQuery( "<div></div>" );
5252

@@ -62,6 +62,10 @@ QUnit.test( "disabling/enabling patches", function( assert ) {
6262
true, "patch enabled by default (proxy)" );
6363
assert.strictEqual( jQuery.migrateIsPatchEnabled( "shorthand-deprecated-v3" ),
6464
true, "patch enabled by default (shorthand-deprecated-v3)" );
65+
assert.strictEqual( jQuery.migrateIsPatchEnabled( "push" ),
66+
true, "patch enabled by default (push)" );
67+
assert.strictEqual( jQuery.migrateIsPatchEnabled( "isArray" ),
68+
true, "patch enabled by default (isArray)" );
6569

6670
expectMessage( assert, "pre-on-methods (default)", function() {
6771
jQuery().bind();
@@ -72,24 +76,47 @@ QUnit.test( "disabling/enabling patches", function( assert ) {
7276
expectMessage( assert, "shorthand-deprecated-v3 (default)", function() {
7377
jQuery().click();
7478
} );
79+
expectMessage( assert, "push (default)", function() {
80+
jQuery().push();
81+
} );
82+
expectMessage( assert, "isArray (default)", function() {
83+
jQuery.isArray();
84+
} );
85+
86+
expectNoMessage( assert, "push access without calling (default)", function() {
87+
assert.strictEqual( typeof jQuery().push, "function",
88+
"access check doesn't trigger a message (push)" );
89+
} );
90+
expectNoMessage( assert, "isArray access without calling (default)", function() {
91+
assert.strictEqual( typeof jQuery.isArray, "function",
92+
"access check doesn't trigger a message (isArray)" );
93+
} );
7594

76-
jQuery.migrateDisablePatches( "pre-on-methods", "proxy" );
95+
jQuery.migrateDisablePatches( "pre-on-methods", "proxy", "push", "isArray" );
7796
assert.strictEqual( jQuery.migrateIsPatchEnabled( "pre-on-methods" ),
7897
false, "patch disabled (pre-on-methods)" );
7998
assert.strictEqual( jQuery.migrateIsPatchEnabled( "proxy" ),
8099
false, "patch disabled (proxy)" );
81100
assert.strictEqual( jQuery.migrateIsPatchEnabled( "shorthand-deprecated-v3" ),
82101
true, "patch still enabled (shorthand-deprecated-v3)" );
102+
assert.strictEqual( jQuery.migrateIsPatchEnabled( "push" ),
103+
false, "patch disabled (push)" );
83104

84-
expectNoMessage( assert, "pre-on-methods (default)", function() {
105+
expectNoMessage( assert, "pre-on-methods (disabled)", function() {
85106
jQuery().bind();
86107
} );
87-
expectNoMessage( assert, "proxy (default)", function() {
108+
expectNoMessage( assert, "proxy (disabled)", function() {
88109
jQuery.proxy( jQuery.noop );
89110
} );
90-
expectMessage( assert, "shorthand-deprecated-v3 (default)", function() {
111+
expectMessage( assert, "shorthand-deprecated-v3 (not disabled)", function() {
91112
jQuery().click();
92113
} );
114+
expectNoMessage( assert, "push (disabled)", function() {
115+
assert.strictEqual( jQuery().push, undefined, "`push` patch no longer defined" );
116+
} );
117+
expectNoMessage( assert, "isArray (disabled)", function() {
118+
assert.strictEqual( jQuery.isArray, undefined, "`jQuery.isArray` patch no longer defined" );
119+
} );
93120

94121
jQuery.migrateDisablePatches( "shorthand-deprecated-v3" );
95122
assert.strictEqual( jQuery.migrateIsPatchEnabled( "shorthand-deprecated-v3" ),

0 commit comments

Comments
 (0)