Add support for async/await, using Babel, and convert some (applicable) code to make use of it
#9944
Conversation
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/6b623451ef99c32/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/6b623451ef99c32/output.txt Total script time: 2.88 mins Published |
There was a problem hiding this comment.
I really like how this works and how it makes the code easier. Especially getting rid of the Promise.resolve().then construction makes it easier to reason about the code. I'm personally in favor of starting to use async/await, especially if we can make it work in the supported browsers. @brendandahl @yurydelendik Thoughts?
| @@ -300,7 +300,7 @@ function setReferer(url, callback) { | |||
| let storageArea = chrome.storage.sync || chrome.storage.local; | |||
|
|
|||
| class ChromePreferences extends BasePreferences { | |||
| _writeToStorage(prefObj) { | |||
| async _writeToStorage(prefObj) { | |||
There was a problem hiding this comment.
Now that this is async, we should be able to remove the return new Promise code a line below. This also applies to the other change in this file and the changes in web/firefoxcom.js.
There was a problem hiding this comment.
Now that this is async, we should be able to remove the return new Promise code a line below.
Nope, that won't work :-)
Please note that, both here and in the other file you refer to, the Promises are resolved from callback functions and that doesn't really work with async/await. (Maybe these shouldn't be marked as async, but that'd look slightly strange since the BasePreferences ones are.)
I really like how this works and how it makes the code easier.
It can (greatly) simplify certain code, as long as you keep in mind that not all Promise usage can/should be replaced with async/await; it really depends on the situation!
Keep in mind that await will suspend the following code, whereas a Promise allows code placed after it to run; these examples are quite good at highlighting the differences.
Snuffleupagus
force-pushed
the
async
branch
11 times, most recently
from
060d4a2
to
7362cd4
Compare
Snuffleupagus
changed the title
async/await, using Babel, and convert some (primarly /web) code to make use of itasync/await, using Babel, and convert some (applicable) code to make use of it
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/0f10e2eac4afc03/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/0f10e2eac4afc03/output.txt Total script time: 6.98 mins Published |
|
Locally on Windows, I get the results as below (all sizes in bytes) for the
The size of the Edit: Note that the latest version of the PR converts code in all major folders (i.e. |
Snuffleupagus
force-pushed
the
async
branch
2 times, most recently
from
62f4019
to
91cd32d
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/0f44a703f0c9d5a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/9ba1c81d0640402/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/9ba1c81d0640402/output.txt Total script time: 28.39 mins
|
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/0f44a703f0c9d5a/output.txt Total script time: 36.21 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/913f175ef4663a3/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/913f175ef4663a3/output.txt Total script time: 7.06 mins Published |
Snuffleupagus
force-pushed
the
async
branch
4 times, most recently
from
d3885a6
to
8c47ddc
Compare
It's most likely presumptuous to assume that this old IRC comment by bdahl implies support for this particular PR; however, it does seem to indicate a wish to be able to use Given that no one has objected "violent" to either using |
|
I'm fine with that. There is also #9521 (comment). I'm glad that Babel seems to handle IE11 for us. I'll land it if I find nothing after another review. |
For proof-of-concept, this patch converts a couple of `Promise` returning methods to use `async` instead. Please note that the `generic` build, based on this patch, has been successfully testing in IE11 (i.e. the viewer loads and nothing is obviously broken). Being able to use modern JavaScript features like `async`/`await` is a huge plus, but there's one (obvious) side-effect: The size of the built files will increase slightly (unless `SKIP_BABEL == true`). That's unavoidable, but seems like a small price to pay in the grand scheme of things. Finally, note that the `chromium` build target was changed to no longer skip Babel translation, since the Chrome extension still supports version `49` of the browser (where native `async` support isn't available).
…r than manually returning `Promise`s This changes the methods signatures of `GenericL10n`, `MozL10n`, and `NullL10n`.
…manually returning `Promise`s
…than manually returning `Promise`s
…ually returning `Promise`s
…rather than manually returning `Promise`s *Ignoring whitespace changes is probably necessary, in order for the diff to be readable.*
…number of Promises and temporary variables necessary when setting the initial document location
|
I'm good with this, I would like to roll it out in chunks though. Can we open a PR with just the first commit? I'll review then we can land and start adding more pieces after a little time to make sure everything is going as expected. |
|
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/1e6d2933dffc657/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/1e6d2933dffc657/output.txt Total script time: 29.50 mins
|
A good introduction to
async/await, explaining the differences toPromises, can be found in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_functionFor proof-of-concept, this patch converts a couple of
Promisereturning methods to useasyncinstead.Please note that the
genericbuild, based on this patch, has been successfully testing in IE11 (i.e. the viewer loads and nothing is obviously broken).Being able to use modern JavaScript features like
async/awaitis a huge plus, but there's one (obvious) side-effect: The size of the built files will increase slightly (unlessSKIP_BABEL == true). That's unavoidable, but seems like a small price to pay in the grand scheme of things.Finally, note that the
chromiumbuild target was changed to no longer skip Babel translation, since the Chrome extension still supports version49of the browser (where nativeasyncsupport isn't available).Much more manageable diff with https://github.com/mozilla/pdf.js/pull/9944/files?w=1