1362970 - Convert .then(null, ...) to .catch(...)

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, enhancement)

Description

[:Paolo Amadini]

We can easily batch convert calls to ".then(null, ...)" and ".then(undefined, ...)" to ".catch(...)", and add an ESLint rule to mandate the use of the latter.

Comment 1

[Florian Quèze [:florian]]

Attachment: script-generated patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b29f2418db17640a28f0975032ec40781e81eb66

Comment 2

[Florian Quèze [:florian]]

Attachment: catch.js xpcshell script

Comment 3

[:Paolo Amadini]

Comment on attachment 8865395 [details] [diff] [review]
script-generated patch

Review of attachment 8865395 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/lib/sdk/l10n/loader.js
@@ +14,5 @@
>  
>  function parseJsonURI(uri) {
>    return readURI(uri).
>      then(JSON.parse).
> +    catch(en(null, function (error) {

...uh?

Comment 4

[Florian Quèze [:florian]]

Attachment: script-generated patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41981bf2f2f9e22724277cd52b11f1520685cd68

Comment 5

[Florian Quèze [:florian]]

Attachment: catch.js xpcshell script v2

(In reply to :Paolo Amadini from comment #3)

> > +    catch(en(null, function (error) {
> 
> ...uh?

That was a bug in my workaround for the broken location of function expressions with a name (I forgot to check that there was a name, so I broke the replacement for anonymous function expression...).

Comment 6

[:Paolo Amadini]

One thing that cannot be detected by the script is that we have some promise implementations like "deprecated-sync-thenables" that don't support "catch". I would actually suggest running a tryserver build and, if anything is still using these libraries, add a basic "catch" method implementation to them.

Comment 7

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #6)

> I would actually suggest running a tryserver build and, if anything is still
> using these libraries, add a basic "catch" method implementation to them.

Feel free to use the builds from my try push in comment 4 to test this.

Comment 8

[:Paolo Amadini]

Comment on attachment 8865399 [details] [diff] [review]
script-generated patch v2

Review of attachment 8865399 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, below are a few things that may be addressed in a fixup patch.

::: dom/crypto/test/test_WebCrypto.html
@@ +495,1 @@
>          );

Might want to inline the ");" in these four instances.

::: dom/promise/tests/unit/test_monitor_uncaught.js
@@ +106,1 @@
>        name: "`then(null, null)`"

`catch(null)`

::: services/fxaccounts/tests/xpcshell/test_oauth_grant_client.js
@@ +104,5 @@
>    };
>  
>    client._Request = new mockResponse(response);
>    client.getTokenFromAssertion("assertion", "scope")
> +    .catch(function(e) {

The function would need a slightly different indentation, not sure it matters though.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +378,2 @@
>      // Fork |promise| to ensure that uncaught errors are reported
> +    return promise.catch(null);

This can just be "return promise.then();"

::: toolkit/components/places/PlacesUtils.jsm
@@ +3029,5 @@
>    undoTransaction: function CLTXN_undoTransaction() {
>      // The getLivemark callback may fail, but it is used just to serialize,
>      // so it doesn't matter.
>      this._promise = PlacesUtils.livemarks.getLivemark({ id: this.item.id })
> +      .catch(null).then( () => {

If the comment is correct this is a bug, and this code was meant to be ".catch(() => {})".

Comment 9

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> > I would actually suggest running a tryserver build and, if anything is still
> > using these libraries, add a basic "catch" method implementation to them.
> 
> Feel free to use the builds from my try push in comment 4 to test this.

Ah, I missed that there were try build links already. Indeed there are failures, so we need to add a new "catch" implementation:

A promise chain failed to handle a rejection: - at chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_auto-pretty-print-02.js:62 - TypeError: closeDebuggerAndFinish(...).catch is not a function [log…]

Funny how these failures were detected by the unhandled rejection code :-)

Comment 10

[Florian Quèze [:florian]]

I discussed this with Paolo over Vidyo, and he'll be handling the next steps here (and I'm still happy to give a hand if needed).

Comment 11

[:Paolo Amadini]

Attachment: Bug 1362970 - Part 3 - Fix indentation and one misuse of "catch".

Review commit: https://reviewboard.mozilla.org/r/149758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149758/

Comment 12

[:Paolo Amadini]

Attachment: Bug 1362970 - Part 1 - Add support for "catch" to "deprecated-sync-thenables".

Review commit: https://reviewboard.mozilla.org/r/149760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149760/

Comment 13

[:Paolo Amadini]

Attachment: Bug 1362970 - Part 2 - Script-generated patch to convert .then(null, ...) to .catch(...).

Review commit: https://reviewboard.mozilla.org/r/149762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149762/

Comment 14

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d9a2bae273099b057b62f22c21984a9098149c

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8878418 [details]
Bug 1362970 - Part 1 - Add support for "catch" to "deprecated-sync-thenables".

https://reviewboard.mozilla.org/r/149760/#review154478

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8878419 [details]
Bug 1362970 - Part 2 - Script-generated patch to convert .then(null, ...) to .catch(...).

https://reviewboard.mozilla.org/r/149762/#review154480

Reviewing a patch of that size is very impactical on mozreview with the pagination... But assuming this is directly the output of the script, r=me with the dom/workers/test/promise_worker.js change reverted.

::: browser/extensions/pdfjs/content/web/viewer.js:1302
(Diff revision 1)
>        return;
>      }
>      this.pdfDocument.getData().then(function getDataSuccess(data) {
>        var blob = (0, _pdfjsLib.createBlob)(data, 'application/pdf');
>        downloadManager.download(blob, url, filename);
> -    }, downloadByUrl).then(null, downloadByUrl);
> +    }, downloadByUrl).catch(downloadByUrl);

Landing changes to browser/extensions/pdfjs requires notifying the pdfjs developers by opening an issue at https://github.com/mozilla/pdf.js/issues

::: browser/extensions/pocket/content/main.js:567
(Diff revision 1)
>      }
>  
>      function getFirefoxAccountSignedInUser(callback) {
>          fxAccounts.getSignedInUser().then(userData => {
>              callback(userData);
> -        }).then(null, error => {
> +        }).catch(error => {

pocket likely also has its own process to accept changes.

::: browser/extensions/screenshots/webextension/background/takeshot.js:50
(Diff revision 1)
>        return browser.tabs.create({url: shot.creatingUrl})
>      }).then((tab) => {
>        openedTab = tab;
>        return uploadShot(shot);
>      }).then(() => {
> -      return browser.tabs.update(openedTab.id, {url: shot.viewUrl}).then(
> +      return browser.tabs.update(openedTab.id, {url: shot.viewUrl}).catch(

and screenshots too.

::: dom/workers/test/promise_worker.js:402
(Diff revision 1)
>    var promise = new Promise(function(resolve, reject) {
>      reject(42);
>    });
>  
>    try {
> -    promise.then(null, function(v) {
> +    promise.catch(function(v) {

The name of the function here (promiseThenNullResolveFunction) makes me think using .then(null was intentional here and should not be changed.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8878417 [details]
Bug 1362970 - Part 3 - Fix indentation and one misuse of "catch".

https://reviewboard.mozilla.org/r/149758/#review154488

Comment 18

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> The name of the function here (promiseThenNullResolveFunction) makes me
> think using .then(null was intentional here and should not be changed.

Good "catch"! ;-)

I've re-generated the patch today and I've reverted the Promise tests and the "extensions" folder. I'm running a tryserver build, and will land the patches if it succeeds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b62320ad45088323d0fe4edc0b1c596e51af0024

Comment 19

[:Paolo Amadini]

Eh, I forgot to exclude "deprecated-sync-thenables" when re-generating the scripted patch this time. New attempt:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d76fb94bc37770b1a60a71656c59e795e68c701

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c210c53f73fc
Part 1 - Add support for "catch" to "deprecated-sync-thenables". r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/1075b94a522d
Part 2 - Script-generated patch to convert .then(null, ...) to .catch(...). r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a9a2014b79
Part 3 - Fix indentation and one misuse of "catch". r=florian

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c210c53f73fc
https://hg.mozilla.org/mozilla-central/rev/1075b94a522d
https://hg.mozilla.org/mozilla-central/rev/e8a9a2014b79