promise.defer in (some) devtools

[Tom Tromey]

As part of the de-chrome-ification project, we're changing how
"promise.defer" is used in some parts of devtools. This is happening in
bug 1273941, landing soon.

Because the promise spec doesn't include "defer", we instead wrote our
own module which should be explicitly required.

Old code:

var promise = require("promise");
var x = promise.defer();

New code:

var defer = require("devtools/shared/defer");
var x = defer();


Affected code, for now, includes devtools/shared, devtools/client/shared,
and the inspector and style editor. Please try not to regress this in
your patches and reviews. At some point we'll probably add an eslint
check for this -- it seemed premature for the time being considering
that we've only converted the bare minimum necessary to bring up the
inspector in content.

This doesn't affect the few modules using deprecated-sync-thenables.

Tom

[Eddy Bruel]

Is the long term plan to get rid of defer altogether? It can always be
rewritten in terms of new Promise(...) { ... }. This does not reduce
legibility in my opinion, and in any case I think it would be good to have
one established way to write promise code.

> _______________________________________________
> dev-developer-tools mailing list
> dev-devel...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-developer-tools
>

[Tom Tromey]

>>>>> "Eddy" == Eddy Bruel <ejpb...@mozilla.com> writes:

Eddy> Is the long term plan to get rid of defer altogether? It can always be
Eddy> rewritten in terms of new Promise(...) { ... }. This does not reduce
Eddy> legibility in my opinion, and in any case I think it would be good to have
Eddy> one established way to write promise code.

I don't remember this idea coming up, and as far as I know, nobody is
planning on it.

I don't have much of an opinion on the merits of the switch. If the
resulting code is just as clear, it would be an improvement, since it
would be more standard. On the other hand, it's a reasonably large
change to make; unless it could be done mechanically it may not be worth
the trouble.

Since we've only converted the inspector and "plausibly
inspector-related" bits of code so far, if we want to make some change
here, it would be good to decide before converting the rest of the tree.
I don't know when that is going to happen.

Tom

[Joe Walker]

I think the "new Promise()" version is to be preferred because it's safer.
If an exception happens during setup, with "new Promise()" you're likely to
get a rejected promise, with "defer()" you run the risk of an unresolved
promise unless you're careful.

Not saying we need to jump to change, but I'm not sure of any reason to
prefer "defer()".

Joe.

[Eddy Bruel]

Good point about new Promise() being safer. I hadn't even considered that.

I don't think this is important enough that we should put any active effort
into making the switch. We could simply disallow the use of deferred in new
code, and apply the boy scout rule to any existing uses.

[Nick Fitzgerald]

In addition to the safety, it is standard and therefore more likely to be
familiar to outside contributors. Without any strong pro-defer arguments, I
think we should add an eslint and switch things over (in an automated
fashion? how much work would it be to add a --fix to the eslint?)

[Eddy Bruel]

I would be surprised if switching over from defer to new Promise(...) could
be done as a simple syntax transformation rule.

On Tue, Jun 28, 2016 at 8:43 PM, Nick Fitzgerald <nfitz...@mozilla.com>
wrote:

[Bryan Clark]

This sounds like a good beginner bug if its really just switching from one
syntax to another. Maybe we file a bug with examples on how to fix it and
a search query of places to be fixed.

[Nick Fitzgerald]

Basically,

function returnsPromise() {
beforeStuff();

let { promise, resolve, reject } = defer();
asyncStuff(function (x) {
if (x) {
resolve(x);
} else {
reject(new Error("expected x");
}
});

return promise;
}

becomes this:

function returnsPromise() {
return new Promise((resolve, reject) => {
beforeStuff();

asyncStuff(function (x) {
if (x) {
resolve(x);
} else {
reject(new Error("expected x"));
});
});

[Jaroslav Šnajdr]

This is a simple change only if the defer.resolve, defer.reject and
defer.promise calls are close together and can be easily wrapped inside the
promise executor function. There are more complex cases where the
refactoring requires some thinking. For example, the maybeResolve and
maybeDone functions at [1] and [2]. It's still easy enough to be a good
first bug, but certainly not an automated search/replace.

[1]
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#271
[2]
https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/head.js#1312

Jarda


On Tue, Jun 28, 2016 at 11:25 PM, Nick Fitzgerald <nfitz...@mozilla.com>
wrote:

[Nick Fitzgerald]

I am pretty sure that wrapping up the continuation after creating the
{promise,resolve,refect} in an explicit function and passing that function
to `new Promise` is always a valid transformation. I guess if the promise
is used in any way besides as a return value, then we would need to save it
in a binding before returning it in the transformed code so that it is
accessible in the continuation function.

But yeah, maybe it is easier to just rewrite it by hand. Depends how much
usage we have in tree.
https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=promise.defer()%20path%3Adevtools
says 527 results. Personally, I think that is over my
want-to-rewrite-by-hand threshold.

[Tom Tromey]

>>>>> "Bryan" == Bryan Clark <cla...@mozilla.com> writes:

Bryan> This sounds like a good beginner bug if its really just switching
Bryan> from one syntax to another. Maybe we file a bug with examples on
Bryan> how to fix it and a search query of places to be fixed.

I've filed this now.
https://bugzilla.mozilla.org/show_bug.cgi?id=1283869

Tom