Replace jQuery.Deferred.done/fail/always()
Closed, ResolvedPublic5 Story Points

Description

In future when Promise's are supported everywhere we will want to use this instead of $.Deferred. We warn against their usage in the code to prevent new introductions but we should fix the existing ones. This will ensure our code remains future proof.

https://caniuse.com/#feat=promises
In preparation for this however, we should deprecate our usage of done, fail and always inside MobileFrontend.

Replace all calls to jQuery.Deferred.done/fail/always(). Watch out:

  • new mw.Api().get().done().abort is defined but new mw.Api().get().then().abort is undefined; XMLHttpRequests may have to be preserved as an intermediate to return an abortable Deferred. See this example.
  • Return the final Deferred, not an intermediate Deferred. e.g.:
ag '\.(done|fail|always)\('
function promiseResultNotReturned() {
  var p = new Promise( (resolve, reject) => resolve() ); 
  p.then( () => 'returning from then');
  return p;
}


function promiseResultReturned() {
  var p = new Promise( (resolve, reject) => resolve() ); 
  var p2 = p.then( () => 'returning from return of then' );
  return p2;
}

var y = promiseResultReturned();
var n = promiseResultNotReturned();

y
/*
Promise (fulfilled: returning from return of then)
*/

n
/*
Promise (fulfilled: undefined)
*/

Deferred methods such as .done(), .fail(), and .pipe() retain their old behavior and so are not Promises/A+ compliant. If you require synchronous resolution, do not want exceptions converted to rejection values or rejection callback returns converted to fulfillment values, or want thrown errors to bubble out of the function where they occur, you can use these methods instead of .then() and .catch()

Current usage:

resources/mobile.editor.api/EditorGateway.js
189:				self.api.postWithToken( 'edit', apiOptions ).done( function ( data ) {
252:				} ).fail( function () {
300:			this._pending = this.api.post( options ).done( function ( resp ) {
317:			} ).fail( function () {

resources/mobile.editor.common/EditorOverlayBase.js
356:				return OO.ui.confirm( mw.msg( 'mobile-frontend-editor-cancel-confirm' ) ).done( function ( confirmed ) {

resources/mobile.editor.overlay/EditorOverlay.js
310:			this.gateway.getPreview( params ).done( function ( result ) {
320:			} ).fail( function () {
322:			} ).always( function () {
469:				.done( function ( result ) {
492:				.fail( function () {
578:				.done( function () {
590:				.fail( function ( data, code, response ) {

resources/mobile.editor.ve/ve.init.mw.MobileFrontendArticleTarget.js
221:		OO.ui.confirm( mw.msg( 'mobile-frontend-editor-switch-confirm' ) ).done( function ( confirmed ) {

resources/mobile.scrollEndEventEmitter/ScrollEndEventEmitter.js
47:	 *           this.gateway.getPhotos().done( function ( photos ) {

resources/mobile.search/SearchOverlay.js
234:			this.router.back().done( function () {

resources/mobile.special.mobilediff.scripts/init.js
36:			} ).done( function ( data ) {
55:			} ).fail( function ( error ) {

resources/mobile.special.nearby.scripts/nearby.js
66:				loader.using( 'mobile.foreignApi' ).done( function () {

resources/mobile.startup/OverlayManager.js
110:						factoryResult.done( function ( overlay ) {

resources/mobile.startup/rlModuleLoader.js
30:			} ).always( function () {

resources/mobile.startup/Skin.js
311:					.done( function () {
330:								gateway.getReferencesList( data.page, id ).done( function ( refListElements ) {
348:					.fail( function () {
353:					.always( function () {

resources/mobile.startup/toast.js
46:			this.notification.done( function ( notif ) {

resources/mobile.talk.overlays/TalkOverlay.js
110:			this.pageGateway.getPage( options.title ).fail( function ( resp ) {
130:			} ).done( function ( pageData ) {

resources/mobile.talk.overlays/TalkSectionAddOverlay.js
133:			this.save().done( function ( status ) {
144:			} ).fail( function ( error ) {

resources/mobile.talk.overlays/TalkSectionOverlay.js
98:			this.pageGateway.getPage( options.title ).done( function ( pageData ) {
135:				} ).done( function () {
141:				} ).fail( function ( data, response ) {
164:				} ).always( function () {

resources/mobile.watchstar/Watchstar.js
172:			gateway.postStatusesByTitle( [ page.getTitle() ], postWatched ).always( function () {
174:			} ).done( function () {
193:			} ).fail( function () {

tests/qunit/mobile.editor.api/test_EditorGateway.js
55:		return gateway.getContent().done( function ( resp ) {
73:		return gateway.getContent().done( function ( resp ) {
94:		gateway.getContent().done( doneSpy ).fail( function ( error ) {
266:		} ).done( doneSpy ).fail( failSpy ).always( function () {
294:		gateway.getContent().done( function () {
296:			gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
332:		gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
363:		gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
398:			gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
433:		gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
466:			gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
492:		gateway.save().done( doneSpy ).fail( failSpy ).always( function () {
561:		} ).done( doneSpy );
590:		} ).done( function ( section ) {
620:		} ).done( function ( section ) {
646:		gateway.save().always( function () {

tests/qunit/mobile.pagelist.scripts/test_WatchstarPageList.js
50:			pageList.getPages( [], [] ).done( function () {
80:			pl.getPages( [ 30, 50 ], [] ).done( function () {

tests/qunit/mobile.references.gateway/test_ReferencesHtmlScraperGateway.js
23:		return this.referencesGateway.getReference( '#cite_note-1', this.page ).done( function ( ref ) {
30:		this.referencesGateway.getReference( '#cite_note-bad', this.page ).fail( function ( err ) {

tests/qunit/mobile.references.gateway/test_ReferencesMobileViewGateway.js
60:		return this.referencesGateway.getReference( '#cite_note-1', this.page ).done( function ( ref ) {
78:		this.referencesGateway.getReference( '#cite_note-bad', this.page ).fail( function ( err ) {
87:		this.referencesGatewayEmpty.getReference( '#cite_note-bad', this.page ).fail( function ( err ) {
96:		this.referencesGatewayRejector.getReference( '#cite_note-bad-2', this.page ).fail( function ( err ) {

tests/qunit/mobile.startup/test_Skin.js
37:		] ).done( function () {
52:		] ).fail( function () {
55:		} ).always( function () {
61:		return this.skin.loadImagesList( [] ).done( function () {
73:		} ).done( function () {

tests/qunit/mobile.startup/test_PageGateway.js
65:		return pageGateway.getPage( 'Test' ).done( function ( resp ) {
178:		pageGateway.getPage( 'Test' ).done( function ( resp ) {
314:		return pageGateway.getPageLanguages( 'Test' ).done( function ( resp ) {
458:		pageGateway.getPage( 'Err' ).fail( function ( msg ) {
493:		return pageGateway.getPage( 'Test' ).done( function ( resp ) {

tests/qunit/mobile.talk.overlays/test_TalkSectionAddOverlay.js
24:		overlay.save().done( function ( status ) {
26:		} ).fail( function ( error ) {

notes

Careful when replacing .done/.fail, specially .fail, as it behaves differently than .catch.

.fail will keep the promise rejected, but adding a .catch` handler will make the promise resolved unless `you throw inside the catch handler again!

See https://github.com/wikimedia/mediawiki-extensions-Popups/blob/e351fd15500e1ab1eb8a7c2e3b975a0d528ece6c/src/actions.js#L137-L138 for an example in popups (that was just a .fail before)

It would be amazing if the owner of this task generates a migration checklist that we can refer to whenever making these changes in other repos.

Migration Checklist

  • For consistency, prefer to use .then( successFn, failFn ) when replacing .done( successFn ).fail( failFn ) or .done( successFn ).catch( failFn ). Be aware of the return value in your callbacks though!
  • In general, .done( function () { ... } ).fail( function () { ... } ).always( alwaysFn ) can be replaced with .then( function () { ... alwaysFn() }, function () { ... alwaysFn() } )
  • Be aware of Deferred handlers with multiple parameters. A Deferred can resolve with multiple arguments passed to its callback functions while a ES6 promise can only resolve with only one argument.
  • When changing the return value of a function, keep in mind that $.Deferred().done() returns a JQuery.Deferred, but $.Deferred().then() returns a JQuery.Promise so you may have to update the JSDocs.
  • You may need to convert tests from sync to async if the tests cover code that previously relied on .done/.fail. This is because .done/.fail callbacks may be called synchronously if the Deferred has already been resolved (which is often the case with test stubs), but .then/.catch callbacks are always called asynchronously. See https://jquery.com/upgrade-guide/3.0/#backwards-compatibility for more info and https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/commit/d42523e609082c300081149f5b9dde1ad6b4ced1#diff-057379d6c79b6705c76c0090f5685011 for an example

Remaining Patches to be Merged

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson updated the task description. (Show Details)Aug 10 2018, 9:09 PM

Temporarily moving into sprint so https://gerrit.wikimedia.org/r/444041 can get reviewed.

Change 444041 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace usage of done in EditorOverlay

https://gerrit.wikimedia.org/r/444041

nray added a subscriber: nray.

Moving this onto the kanbanana-board even though it is the second from top in Upcoming because I think I can be more effective on this than T199157 (top one)

nray claimed this task.

Change 452834 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then

https://gerrit.wikimedia.org/r/452834

Change 452846 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in CategoryAddOverlay

https://gerrit.wikimedia.org/r/452846

Change 452849 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in uploads.js

https://gerrit.wikimedia.org/r/452849

Change 452846 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in CategoryAddOverlay

https://gerrit.wikimedia.org/r/452846

Change 452834 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in CategoryOverlay

https://gerrit.wikimedia.org/r/452834

Change 452849 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in uploads.js

https://gerrit.wikimedia.org/r/452849

Given we support IE11, we cannot do this right?
https://caniuse.com/#feat=promises

Beware that the default view there only shows current versions. We also support Grade A for Android 4, iOS 6-8, Safari 5-8, and Opera 15-20; which also predate Promise. Promise is new in ES6, so we'll probably start their use when we require ES6 (T178356).

@Krinkle sure. This was a clarifying question (with illustration) that Stephen answered. These changes are purely to prepare us, nobody is proposing switching to Promises right now.

Jdlrobson updated the task description. (Show Details)Aug 15 2018, 11:15 PM

Change 453451 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail/.always with .then in TalkOverlay et al.

https://gerrit.wikimedia.org/r/453451

Change 453456 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in Watchstar/WatchstarPageList

https://gerrit.wikimedia.org/r/453456

Change 453458 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in ve.init.mw.MobileFrontendArticleTarget

https://gerrit.wikimedia.org/r/453458

Change 453459 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in nearby.js

https://gerrit.wikimedia.org/r/453459

Change 453460 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in OverlayManager

https://gerrit.wikimedia.org/r/453460

Change 453461 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then/.catch in PageGateway Test

https://gerrit.wikimedia.org/r/453461

Change 453462 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then/.catch in mobile.references.gateway tests

https://gerrit.wikimedia.org/r/453462

Change 453464 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in ScrollEndEventEmitter.js

https://gerrit.wikimedia.org/r/453464

Change 453465 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in SearchOverlay

https://gerrit.wikimedia.org/r/453465

Change 453466 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in Skin.js

https://gerrit.wikimedia.org/r/453466

Change 453467 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in mobile.special.mobilediff.scripts/init.js

https://gerrit.wikimedia.org/r/453467

Change 453468 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove .always in rlModuleLoader

https://gerrit.wikimedia.org/r/453468

Change 453469 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in toast.js

https://gerrit.wikimedia.org/r/453469

Change 453475 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail/.always with .then/.catch in mobile editor

https://gerrit.wikimedia.org/r/453475

Change 453478 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Eslint: forbid use of done, always and fail

https://gerrit.wikimedia.org/r/453478

Change 453458 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in ve.init.mw.MobileFrontendArticleTarget

https://gerrit.wikimedia.org/r/453458

Change 453459 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in nearby.js

https://gerrit.wikimedia.org/r/453459

Change 453469 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in toast.js

https://gerrit.wikimedia.org/r/453469

Change 453468 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove .always in rlModuleLoader

https://gerrit.wikimedia.org/r/453468

Change 453460 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in OverlayManager

https://gerrit.wikimedia.org/r/453460

Change 453456 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in Watchstar/WatchstarPageList

https://gerrit.wikimedia.org/r/453456

Change 453464 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in ScrollEndEventEmitter.js

https://gerrit.wikimedia.org/r/453464

Change 453462 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then/.catch in mobile.references.gateway tests

https://gerrit.wikimedia.org/r/453462

Change 453467 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in mobile.special.mobilediff.scripts/init.js

https://gerrit.wikimedia.org/r/453467

nray updated the task description. (Show Details)Aug 20 2018, 4:45 PM

Change 453465 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done with .then in SearchOverlay

https://gerrit.wikimedia.org/r/453465

Change 453466 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then in Skin.js

https://gerrit.wikimedia.org/r/453466

@nray I've left two -1 and one rebase request to complete code review here.

nray updated the task description. (Show Details)Aug 20 2018, 10:15 PM

Change 453451 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail/.always with .then/.catch in TalkOverlay et al.

https://gerrit.wikimedia.org/r/453451

Change 454439 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Forbid use of done/always/fail

https://gerrit.wikimedia.org/r/454439

Change 453461 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail with .then/.catch in PageGateway Test

https://gerrit.wikimedia.org/r/453461

Change 454441 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Make Deferreds promise compatible

https://gerrit.wikimedia.org/r/454441

nray updated the task description. (Show Details)Aug 22 2018, 12:27 AM

Change 453475 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Replace .done/.fail/.always with .then/.catch in mobile editor

https://gerrit.wikimedia.org/r/453475

Jdlrobson updated the task description. (Show Details)Aug 22 2018, 3:00 PM
Jdlrobson updated the task description. (Show Details)

Change 453478 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Eslint: forbid use of done, always and fail

https://gerrit.wikimedia.org/r/453478

nray updated the task description. (Show Details)Aug 22 2018, 10:21 PM
nray updated the task description. (Show Details)Aug 22 2018, 10:26 PM

Change 454441 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make Deferreds promise compatible

https://gerrit.wikimedia.org/r/454441

nray updated the task description. (Show Details)Aug 22 2018, 11:05 PM

Change 454439 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Forbid use of done/always/fail

https://gerrit.wikimedia.org/r/454439

Jdlrobson reassigned this task from nray to Niedzielski.Aug 23 2018, 1:22 AM
Jdlrobson added subscribers: alexhollender, ovasileva.

I'm skipping QA+design review on this one as the QA requirements are quite broad and during retrospective we talked about how it was hard to QA these ones. No CSS should be impacted, but there is a possibility that error handling within certain overlays may be impacted - but during code review extra care was taken to account for that not being the case @alexhollender @ovasileva a general QA may be useful (given we have many tasks like this in sprint), but I don't think it's essential. We might want to cut a specific card for QA if that makes you more comfortable. I tell you this more to ensure you are aware and if you notice anything let us know.

I've documented @nray's checklist over here > https://www.mediawiki.org/w/index.php?title=Reading/Web/Working_with_legacy_code and linked to it from https://www.mediawiki.org/w/index.php?title=Reading/Web/Team

@Niedzielski do you want to sign this one off as the original proposer?

nray added a comment.Aug 23 2018, 2:18 AM

To whoever is signing off on this: It should be noted that there is one remaining .fail left in EditorGateway.js (MobileFrontend) and one test in test_EditorGateway.js which still uses .done/.fail/.always. These have been discussed and decided to be worked on in https://phabricator.wikimedia.org/T202374.

@Niedzielski is off so Jan will sign off!

Jdlrobson raised the priority of this task from Normal to High.
Jdrewniak closed this task as Resolved.Aug 30 2018, 8:55 AM

great job guys!