Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

Refactor .done() and .fail() to .then()
Closed, ResolvedPublic

Description

Using .done() and .fail() can lead to some nasty bugs such as T238025. Refactor these to .then().

https://www.mediawiki.org/wiki/Manual:Coding_conventions/JavaScript#Asynchronous_code

Avoid using jQuery-specific methods like done() or fail(), which could stop working without warning if the method you are calling internally transitions from $.Deferred to native Promise.

Similar to what we did in this patch.

Thanks to @Chlod for pointing this out

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

This does also seem like a good stepping stone for two other things: refactoring the existing functions to make them resolve a success/failure, which will in turn allow us to create .then chains instead of calling functions within other functions (in what's essentially callback hell but with promises), and making things run asynchronously to boost performance. The latter is more of a "what if" that greatly improves client-side processing time (since we no longer wait for everything to happen in succession), but comes at the expense of a task in the chain not having happened yet (e.g. the AfD page not having been created yet but the page already being tagged with the AfD template) either due to a slow internet connection or an API error (both of which, luckily, can be redone or caught for user action).

In any case, this doesn't seem to be that big of a task (codesearch doesn't show a lot of uses) so I'm willing to work on this (and the proposed two ideas above, either just the first or both, with your input/blessing), @Novem_Linguae.

@Chlod, thanks so much for stepping up. Yes, feel free to work on this.

As for whether we should try to make complicated functions asynchronous, in my opinion linear code is more readable and less prone to bugs/race conditions, and this sacrifice is worth some slowness. In my opinion we should maximize readability and lack-of-bugs over speed. If we could use ES9 async/await, I would probably refactor that delete.js -> submit() function to something like...

async function submit() {
    try {
        // before stuff
        
        await ifXFDWriteToLog();
        await ifXFDWriteXFDPage();
        await tagArticle();
        await writeToUserTalkPage();
        
        // after stuff
    } catch {
        // fatal error that doesn't execute any more steps
    }
}

With that said, if you think you can convert all that to async, make it readable, and make it bug free, I'm open to that as well. You are clearly a guru when it comes to promises :)

If we could use ES9 async/await, I would probably refactor that delete.js -> submit() function to something like...

You can, if you transpile it back to ES6 before committing. ES6 is the base line for all advanced and non-critical Javascript code of Wikipedia. (you have to manually mark the module as es6, otherwise the baseline is es5).

Transpilation carries its own complications and setting it up would lead to bikeshedding (especially with more pressing matters at hand). It'll also introduce boilerplate code that doesn't look good. A linear way of writing the promise chain does exist in plain ES5+JQuery:

{
    // ...
    submit: function () {
        // ...
        // modify actionQueue with whatever needs to be done...
        that.logPage( tagObj )
            .then( that.discussionPage.bind( that, tagObj ) )
            .then( that.tagPage.bind( that ) ) // resolves with { tagCount: count, tagKey: key }
            // Theoretically, I could avoid using a closure entirely by changing `notifyUser`'s signature to
            // match the output of `tagPage`, but I'd prefer keeping that function's existing signature.
            // `.bind` also won't work here, since the arguments we need are not in the current context.
            .then( function ( data ) {
                return that.notifyUser.call( this, data.tagCount, data.tagKey );
            }.bind( that ) )
            // Function below doesn't use `this` anyway, so just set `this` context for it to null.
            .then( mw.pageTriage.actionQueue.runAndRefresh.bind( null, actionQueue, that.getDataForActionQueue() ) )
            .catch( that.handleError.bind( that ) );
        // ...
    }
    // ...
}

Though this is pseudocode, I've done my best to preserve function signatures, just in case there's undocumented usages for those. Assume that all the functions above throw new Error( mw.msg( 'pagetriage-error-message-id-here' ) ); on error (which feeds all errors to that.handleError that we have at the bottom, for further processing). Compared to what we have right now, this is as clean as it gets (without ES9 or transpilation) and now we don't have callback hell.
The usage of .bind() here may be useful in the future; I noticed a lot of that = thising (i.e. juggling this) which is usually resolved by using Function.prototype.bind. It's set to that for now, so that Find+Replace would jive much better with it on the day that we finally bind statements so that we don't have this context issues.

Change 840392 had a related patch set uploaded (by Chlod Alejandro; author: Chlod Alejandro):

[mediawiki/extensions/PageTriage@master] Promise cleanup

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

Chlod moved this task from Soon to Code Review on the PageTriage board.

Change 840392 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Promise cleanup

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