Ajax: Warn against automatic JSON-to-JSONP promotion #376
Conversation
| return expectNoWarning( assert, "dataType: \"json\"", | ||
| function() { | ||
| return jQuery.ajax( { | ||
| url: url( "data/null.json" ), |
There was a problem hiding this comment.
This file just contains null which is both valid JSON & JS. Quite handy!
| // Invoke tests sequentially as they're async and early tests could get warnings | ||
| // from later ones. | ||
| function run( tests ) { | ||
| var test = tests[ 0 ]; | ||
| return test().then( function() { | ||
| if ( tests.length > 1 ) { | ||
| return run( tests.slice( 1 ) ); | ||
| } | ||
| } ); | ||
| } ); | ||
| } | ||
|
|
||
| run( tests ) | ||
| .then( function() { | ||
| done(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
This is quite a lot of boilerplate but we don't have that many tests so I'm not sure if this is the moment to generalize it... If there's a way to simplify, I'm all ears. :)
There was a problem hiding this comment.
I'm fine with it, if it really gets out of hand we can refactor some other time.
| <script src="testinit.js"></script> | ||
| <script> | ||
| TestManager.loadProject( "jquery", "git" ); | ||
| TestManager.loadProject( "jquery", "3.x-git" ); |
There was a problem hiding this comment.
I can separate these changes if you want.
| // Register this prefilter before the jQuery one. Otherwise, a promoted | ||
| // request is transformed into one with the script dataType and we can't | ||
| // catch it anymore. | ||
| jQuery.ajaxPrefilter( "+json", function( s ) { |
There was a problem hiding this comment.
The special + handling is not documented at https://api.jquery.com/jQuery.ajaxPrefilter/, I found it by reading source. 🙈
| // Invoke tests sequentially as they're async and early tests could get warnings | ||
| // from later ones. | ||
| function run( tests ) { | ||
| var test = tests[ 0 ]; | ||
| return test().then( function() { | ||
| if ( tests.length > 1 ) { | ||
| return run( tests.slice( 1 ) ); | ||
| } | ||
| } ); | ||
| } ); | ||
| } | ||
|
|
||
| run( tests ) | ||
| .then( function() { | ||
| done(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
I'm fine with it, if it really gets out of hand we can refactor some other time.
test/migrate.js
Outdated
| } | ||
|
|
||
| if ( result && result.then ) { | ||
| return jQuery.when( |
There was a problem hiding this comment.
Note to self - jQuery.when is unavailable in jQuery 4.x Slim. Perhaps it's better to use a Promise polyfill like native-promise-only that we already use in Core tests.
|
@dmethvin I added one commit switching |
dmethvin
left a comment
dmethvin
left a comment
There was a problem hiding this comment.
Seems like the easiest way to make this work in slim, LGTM.
The warning landed in jquerygh-376 but I forgot to document it. Fixes jquerygh-443 Ref jquerygh-376
The warning landed in gh-376 but I forgot to document it. Fixes gh-443 Closes gh-447 Ref gh-376 Co-authored-by: Dave Methvin <[email protected]>
3 participants
Fixes gh-372
Ref jquery/jquery#1799
Ref jquery/jquery#3376
Ref jquery/jquery#4754