Use .done() instead of .then() when adding success-handlers on Promise objects
Closed, ResolvedPublic
Actions

Assigned To

None

Authored By

	Krinkle
	Jun 27 2012, 8:27 AM

Description

Hi,

I didn't know where else to put it so I'll write it here. When randomly looking through a diff on Gerrit I noticed a few mentions of this the following (it wasn't changed in that commit, but shown in the diff context):

» this.performApiAction( action )

» » .then( degrade );

» var deferred = this.performApiAction( this.API_ACTION.SAVE )

» » .then( $.proxy( ... ) );

etc.

.then is literally just this:

then: function( doneFns, failFns, progressfns ) {
deferred.done( doneFns ).fail( failFns ).progress( progressfns );
return this;
},

So just use .done(). Even when binding both 'done' and 'fail' at the same time, I'd still recommend using them directly for clarity, but that's just me :)

Version: unspecified
Severity: trivial
URL: https://gerrit.wikimedia.org/r/gitweb?p=mediawiki%2Fextensions%2FWikidataRepo.git&a=search&h=HEAD&st=grep&s=.then

Details

Reference: bz37982

Event Timeline

• bzimport raised the priority of this task from to Low.Nov 22 2014, 12:30 AM

• bzimport added projects: MediaWiki-extensions-WikibaseRepository, JavaScript, patch-welcome.

• bzimport set Reference to bz37982.

• bzimport added a subscriber: Unknown Object (MLST).

Krinkle created this task.Jun 27 2012, 8:27 AM

I agree, especially using .then() with two arguments is not very readable. It's pretty easy to mistake then for done or the other way around.

Note that es-harmony seems to be standardizing promises in such a way they'll eventually be a standard feature.

Depending on the strawman that gets standardized it will either be .then(doneFn, failFn); or .when(doneFn, failFn);

Though these promises aren't going to have something like .progress().

(In reply to comment #2)

Note that es-harmony seems to be standardizing promises in such a way they'll
eventually be a standard feature.

Depending on the strawman that gets standardized it will either be
.then(doneFn, failFn); or .when(doneFn, failFn);

Though these promises aren't going to have something like .progress().

Interesting, but lets not fall into the pitfall of bad coding patterns under the excuse of familiar patterns from unrelated environments.

We are using a jQuery promise, which should be used the way a jQuery promise should be used. Even if that means it won't look "like" an es-harmony promise.

I'm not saying .then() will fail on a jQuery promise, but that's not how I think we should encourage usage of it.

At least what es-harmony will or will not do can not possibly play a role in this decision.

changed accordingly: https://gerrit.wikimedia.org/r/#/c/17794/ -- by an external volunteer developer! Yay Joancreus!

Verified in Wikidata demo time for sprint 11

Use .done() instead of .then() when adding success-handlers on Promise objectsClosed, ResolvedPublicActions