refactor: use async/await syntax #411

rpl · 2016-08-04T14:31:38Z

The goal of this PR is discussing the following proposals:

introducing async / await syntax to make some of the asynchronous functions nicer to read
introducing more explicit flow types declarations to extend the kind of statical analysis that flow is able to
make on the sources

The first patch contains a set of experimental changes related to the usage of the async / await syntax to make long chains of promises more readable, by configuring babel to transpile them into code that can run across the various supported nodejs versions.
This patch ports to the async / await syntax only functions where it makes the code nicer and easier to read, and it doesn't port to the new syntax any of the test cases (mostly because it is better to keep the tests unmodified during such a change).

The second patch introduces more explicit flow types to be able to give to flow the knowledge needed to check the code in much more detail, e.g. by declaring the types of the objects passed as params, signature of the callbacks etc.
The second patch can be augmented with much more explicit flow types definitions, but I wanted to give an initial taste of what it means in terms of the introduced changes before going further in this path. (landed as #424)

As a side note, async/await has been recently moved into Stage 4, and so it seems that it will be officially part of the 2017 version of the standard.

coveralls · 2016-08-04T14:35:44Z

Coverage remained the same at 100.0% when pulling 8068adb on rpl:refactor/async-await into c73c315 on mozilla:master.

coveralls · 2016-08-18T17:34:55Z

Coverage decreased (-0.2%) to 99.837% when pulling ea8700e on rpl:refactor/async-await into 6654239 on mozilla:master.

rpl · 2016-08-18T17:43:03Z

@kumar303 I've just updated this PR by rewriting any function that could benefit from the new async/await syntax, above the changes introduced by #424

The coverall report is currently red because of a small helper that I added into "utils/errors.js", I'm going to write a new small test for it asap.

Besides the (temporary) coverall report failure, this PR should now ready for a review.

coveralls · 2016-08-18T19:04:11Z

Coverage remained the same at 100.0% when pulling 406aadc on rpl:refactor/async-await into 6654239 on mozilla:master.

rpl · 2016-08-18T19:06:17Z

@kumar303 I just added the new test case for the isErrorWithCode helper (as 406aadc) which makes coveralls to be green and happy as before ;-)

kumar303 · 2016-08-18T20:46:58Z

src/cmd/build.js

+    manifestData = await getValidatedManifest(sourceDir);
+  }
+
+  let buffer =  await zipDir(sourceDir, {


nit: extra space after the equal sign. Maybe we could add a linting rule for that?

I think the rule in question is no-multi-spaces.

👍

added no-multi-spaces to the enabled eslint rules and removed all the related linting errors in 358c26b

kumar303 · 2016-08-18T20:53:55Z

What happens if some code accidentally calls an async function without using the await keyword? Is there an error? If not, could we add a linting rule for that?

kumar303 · 2016-08-18T20:56:57Z

src/cmd/run.js

+      try {
+        return await connectToFirefox();
+      } catch (error) {
+        if (isErrorWithCode('ECONNREFUSED', error)) {


👍 for isErrorWithCode(), that reads well

kumar303 · 2016-08-18T21:34:57Z

I'm excited about this! I got halfway through but I have to end for the day. I'll pick it up tomorrow.

coveralls · 2016-08-19T13:54:35Z

Coverage remained the same at 100.0% when pulling 358c26b on rpl:refactor/async-await into 6654239 on mozilla:master.

rpl · 2016-08-19T14:14:56Z

What happens if some code accidentally calls an async function without using the await keyword? Is > there an error? If not, could we add a linting rule for that?

@kumar303 sorry, I forgot to mention this in the summary of this pull request, and I totally agree that it is an important detail that needs to be discussed in this pull request:

An "async function" always returns a Promise, the returned promise is resolved to the value return-ed by the async function, if an exception is thrown in the body of the async function (and there is no try/catch to stop it), then the returned promise is rejected and the exception passed as parameter of the Promise error callback.

This is very nice, because it means that async function can be composed into Promise chains and mixed with non-async function which returns a Promise (e.g. it is completely valid to put the result of an async function into a Promise.all array), but it probably makes a bit harder to decide if the await keyword is missing by mistake or intentionally, e.g.:

let res = asyncFunction01(...);
let res2 = asyncFunction02(...);

await Promise.all([res, res2]);

versus

let res = await asyncFunction01(...);
...
let res2 = await asyncFunction02(...);
...

Nevertheless, there is an eslint-plugin-async-await npm package that has two rules that can be interesting, because they enforce a space where async/await can be confused for a function name, e.g.:

// This is valid for babel, but it can be confusing
let res = await(asyncFunction01(...)); 

// is what the `eslint-plugin-async-await` can help to enforce
let res = await (asyncFunction01(...));

rpl · 2016-08-19T14:25:23Z

but it probably makes a bit harder to decide if the await keyword is missing by mistake or intentionally

to be completely fair, what I think is that probably eslint will not be able to detect this kind of mistakes... but I expect that most of them should be caught by the flow type checks :-D

e.g. Flow should be able to detect when we are trying to use a promise as the result of "awaiting a promise resolved value", as well as if we are trying to put into a Promise.all something that is not a promise, and if it doesn't detect it, now we know how to look into the flow coveraging info ;-)

coveralls · 2016-08-19T15:30:06Z

Coverage remained the same at 100.0% when pulling ff12c02 on rpl:refactor/async-await into 6654239 on mozilla:master.

coveralls · 2016-08-19T15:57:04Z

Coverage remained the same at 100.0% when pulling cdcbc92 on rpl:refactor/async-await into 6654239 on mozilla:master.

coveralls · 2016-09-15T11:57:48Z

Coverage remained the same at 100.0% when pulling bdb10bf on rpl:refactor/async-await into 0d1b824 on mozilla:master.

…ustom range generator.

…async function.

coveralls · 2016-09-15T17:54:40Z

Coverage remained the same at 100.0% when pulling 4903d57 on rpl:refactor/async-await into 7fac094 on mozilla:master.

rpl · 2016-09-15T18:00:06Z

src/firefox/index.js

+
+  let client;
+
+  while (retriesLeft >= 0) {


@kumar303 This is the "async/await"-based version of the new defaultRemotePortFinder (which is the bigger change after the rebase, and it worth a second look from an additional pair of eyes ;-))

This looks great to me.

kumar303 · 2016-09-15T18:44:29Z

src/firefox/index.js

+    try {
+      client = await connectToFirefox(portToTry);
+      log.debug(`Remote Firefox port ${portToTry} is in use ` +
+                `(retries remaining: ${retriesLeft} )`);


while you're at it, you could fix the extra space in the parentheses around ${retriesLeft} :) no biggie though, just looks odd in the console.

👍 small tweak just pushed

coveralls · 2016-09-15T18:54:38Z

Coverage remained the same at 100.0% when pulling e90c497 on rpl:refactor/async-await into 7fac094 on mozilla:master.

muffinresearch added the state: pull request ready label Aug 4, 2016

kumar303 mentioned this pull request Aug 5, 2016

feat: Add file extension option to web-ext build #392

Closed

rpl mentioned this pull request Aug 8, 2016

Test: add more flow types #424

Merged

rpl force-pushed the refactor/async-await branch from 8068adb to ea8700e Compare August 18, 2016 17:29

rpl assigned kumar303 Aug 18, 2016

kumar303 reviewed Aug 18, 2016
View reviewed changes

rpl changed the title ~~refactor: use async/await and add more explicit flow types declarations~~ refactor: use async/await syntax Aug 19, 2016

rpl force-pushed the refactor/async-await branch from 992cd56 to bdb10bf Compare September 15, 2016 11:50

rpl added 19 commits September 15, 2016 19:47

refactor: enabled babel runtime in babel config.

5a45221

refactor: ported cmd/build.js to async/await syntax.

0719a15

refactor: ported cmd/run.js to asyn/await syntax.

1b026c2

refactor: ported cmd/sign.js to async/await syntax.

a12431e

refactor: ported firefox/index.js to async/await syntax.

3294a1a

refactor: ported firefox/remote.js to async/await syntax.

94b45dd

refactor: ported util/artifacts.js to async/await syntax.

174dc85

refactor: ported util/manifest.js to async/await syntax.

38d890a

refactor: ported program.js to async/await syntax.

0c44ad9

test: added test case for the new isErrorWithCode helper.

fa1cd2a

test: added no-multi-spaces eslint rule, and fixed related lint errors

3df145c

test: added eslint-plugin-async-await plugin and rules

dea4c37

refactor: removed promisedStream var from defaultPackageCreator.

b2b4439

refactor: rewrote establishConnection helper using a for loop and a c…

6adbc3c

…ustom range generator.

style: changed a multiline string into a single line.

b8a37f5

style: tweaked try/catch blocks in cmd/sign and firefox/index modules.

b340f9f

test: enabled require-yield eslint rule to enforce await usage in an …

eca3847

…async function.

refactor: change non-reassigned identifier to const in src/program.

8f4c0cd

refactor: revert generator usage in for loop.

4903d57

rpl force-pushed the refactor/async-await branch from bdb10bf to 4903d57 Compare September 15, 2016 17:48

rpl commented Sep 15, 2016

View reviewed changes

kumar303 reviewed Sep 15, 2016

View reviewed changes

style: remove extra space in a log message printed to console.

e90c497

kumar303 merged commit 4c530df into mozilla:master Sep 15, 2016

kumar303 removed the state: pull request ready label Sep 15, 2016

kumar303 mentioned this pull request Sep 20, 2016

Convert all of web-ext promise chains to coroutines #72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use async/await syntax #411

refactor: use async/await syntax #411

rpl commented Aug 4, 2016 •

edited

coveralls commented Aug 4, 2016 •

edited

coveralls commented Aug 18, 2016 •

edited

rpl commented Aug 18, 2016

coveralls commented Aug 18, 2016 •

edited

rpl commented Aug 18, 2016

kumar303 Aug 18, 2016

pdehaan Aug 18, 2016

rpl Aug 19, 2016

kumar303 commented Aug 18, 2016

kumar303 Aug 18, 2016

kumar303 commented Aug 18, 2016

coveralls commented Aug 19, 2016 •

edited

rpl commented Aug 19, 2016

rpl commented Aug 19, 2016

coveralls commented Aug 19, 2016 •

edited

coveralls commented Aug 19, 2016 •

edited

coveralls commented Sep 15, 2016 •

edited

coveralls commented Sep 15, 2016 •

edited

rpl Sep 15, 2016

kumar303 Sep 15, 2016

kumar303 Sep 15, 2016

rpl Sep 15, 2016

coveralls commented Sep 15, 2016 •

edited

refactor: use async/await syntax #411

refactor: use async/await syntax #411

Conversation

rpl commented Aug 4, 2016 • edited

coveralls commented Aug 4, 2016 • edited

coveralls commented Aug 18, 2016 • edited

rpl commented Aug 18, 2016

coveralls commented Aug 18, 2016 • edited

rpl commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Aug 18, 2016

Choose a reason for hiding this comment

kumar303 commented Aug 18, 2016

coveralls commented Aug 19, 2016 • edited

rpl commented Aug 19, 2016

rpl commented Aug 19, 2016

coveralls commented Aug 19, 2016 • edited

coveralls commented Aug 19, 2016 • edited

coveralls commented Sep 15, 2016 • edited

coveralls commented Sep 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 15, 2016 • edited

rpl commented Aug 4, 2016 •

edited

coveralls commented Aug 4, 2016 •

edited

coveralls commented Aug 18, 2016 •

edited

coveralls commented Aug 18, 2016 •

edited

coveralls commented Aug 19, 2016 •

edited

coveralls commented Aug 19, 2016 •

edited

coveralls commented Aug 19, 2016 •

edited

coveralls commented Sep 15, 2016 •

edited

coveralls commented Sep 15, 2016 •

edited

coveralls commented Sep 15, 2016 •

edited