1329116 - Update TelemetryEnvironment.jsm to async function & await

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

We have `async function` and `await` now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

We should be able to switch all of the `Task.async`, `Task.spawn` and `function*` uses over to those:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2F+ext%3Ajs+ext%3Ajsm+Task.&redirect=false

The only case i can think of that doesn't migrate trivially is the `DeferredTask` usage, we could just leave that one for now.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I would be happy to mentor this. Note though that this isn't a good first bug.

Changes can be checked against our unit tests using:
mach xpcshell-test toolkit/components/telemetry/tests/unit

To start with a smaller change, you can make changes to e.g. only TelemetryEnvironment.jsm and run only the test for that:
mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.jsm

After the tests runs through fine, we also need to manually test by running the Firefox build:
- check that pings are still generated as expected on e.g. addon installs, using about:telemetry
- sent out successfully, using https://github.com/mozilla/gzipServer
- no new errors related to our code should show up in the browser console

Comment 2

[Aman Dwivedi]

Hi Georg! I have taken up a few good first bugs till now. I would like to take this up. Can you please assign it to me and help me get started with this?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Sure - the relevant information is in comment 0 & comment 1. We want to get rid of the mentioned `Task.*` and `function*` uses and instead use `async` & `await` instead.

As mentioned, i really recommend starting with only the TelemetryEnvironment files to see how it works.
Let us know if you run into any problems or have questions.

Comment 4

[Deepjyoti Mondal]

Attachment: bug1329116.patch

I am trying to solve this issue. I have started with the TelemetryEnvironment.jsm file. I am facing some problems.

I have changed Task.async(function*() { /*stuff*/ }) to 
async function() { /*stuff*/ }

however I am getting the following error (below attached file)

Comment 5

[Deepjyoti Mondal]

Attachment: error_test.text

Comment 6

[Georg Fritzsche [:gfritzsche]]

The actual error line is:
> ERROR SyntaxError: yield is a reserved identifier at resource://gre/modules/TelemetryEnvironment.jsm:530

This is because `yield` is only allowed in generator functions [1], which were used with Task.async() etc. to wait on promises.
With the change you did, you need to use `await` instead of `yield`.

Comment 7

[Georg Fritzsche [:gfritzsche]]

To reduce the scope of this bug, i'm limiting this to only changing TelemetryEnvironment.jsm.

Comment 8

[Deepjyoti Mondal]

Attachment: bug1329116.patch

I have changed yield to await, now there is no more errors

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8844321 [details] [diff] [review]
bug1329116.patch

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

Thanks! This looks good, with the eslint error below fixed.

Lets also remove the import for Task.jsm at the start of the file. With the changes here it is no longer needed.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +516,4 @@
>     *   changed - Whether the environment changed.
>     *   oldEnvironment - Only set if a change occured, contains the environment data before the change.
>     */
> +  _updateAddons: async function() {

Please make sure to fix the eslint errors, e.g. from running:
./mach eslint toolkit/components/telemetry/

This can now use the shorthand async method definitions:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods

Comment 10

[Deepjyoti Mondal]

Attachment: bug1329116.patch

Removed the eslint error

Comment 11

[Deepjyoti Mondal]

Attachment: bug1329116.patch

fixed eslint errors and removed the import statement for Task.jsm

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8844994 [details] [diff] [review]
bug1329116.patch

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

Thanks, this looks good!

Comment 13

[Georg Fritzsche [:gfritzsche]]

I've run the tests locally, this should be good to land.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2eaf534ed7
Update TelemetryEnvironment.jsm to async function & await. r=gfritzsche

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4c2eaf534ed7