Addons-frontend: Move all promises to async / await syntax

[kumar303]

Promises can be very hard to read, especially when we need to do specific exception handling that should not affect the rest of the promise chain. Let's consider fully moving to async / await instead.

You can grep for \.then in src to get an idea of where there are still promises. You will notice a few examples where we will still need to use promises, such as a top-level call like createClient().

Contributors: I recommend beginning with a small patch where you only fix one or two instances.

[aayushsanghavi]

@kumar303 I would like to take a shot at this. Could you tell me where all we need to change the code to async-await?

[tofumatt]

You would need to convert all Promises code over to the async/await syntax. This might also involve learning flow given we annotate some Promises code with flow. But going through the Promises code would be a good start, as that's what we want to convert to use async/await.

[aayushsanghavi]

Could you link some files that use promises?

[tofumatt]

Unfortunately I won't be able to link to all the files; a fair bit of the work in this patch is the task of identifying all the Promises code–once I'd have found them all it would be easier for me to complete the patch than link to all the files.

You can look through the codebase with an editor like Atom for Promises code (a good sign in .then(.

Are you familiar with Promises and with async/await?

[aayushsanghavi]

Haha okay. I'll look up the files first, then.
Neither am I familiar with async/await nor with promises but I hope to learn about them through this issue. I'll read up on it from the link @kumar303 left in the description. He also left a link to an issue he fixed in the web-ext repository which is quite similar to this one. The changes he made will be useful for reference. Then I'll try working on the code and file a PR if possible. If it doesn't work out, I'll drop it,

Sounds good?

[tofumatt]

That sounds fine, yup! Always feel free to try working on something, cheers.

[aayushsanghavi]

Hey @tofumatt we've got to look for .then in the src directory only right? I happened to search it in the root directory of the project and got an astonishingly huge number of results. I guess thats not what we want right?

[tofumatt]

src/ and tests/ at least, I'm not sure if we use them elsewhere. You'll want to ignore node_modules, obviously, as it will be full of third-party libraries using Promises.

[Razaik25]

If no one is working on this, can i give this a try?

[muffinresearch]

@aayushsanghavi had you made any progress with this?

I would definitely say starting on this issue requires solid knowledge of Promises and how they work since Async/Await is built on top of them. Not wanting to put anyone off but there might be better other bugs that are a bit easier to get traction on.

[tofumatt]

This will also be a very large diff that will take us a bit of time to review and will bitrot fast, so it's best to do this in Q4.

Matthew Riley MacPherson (sent from mobile)

On 14 Sep 2017, at 21:18, Stuart Colville [e-mail] wrote:

@aayushsanghavi had you made any progress with this?

I would definitely say starting on this issue requires solid knowledge of Promises and how they work since Async/Await is built on top of them. Not wanting to put anyone off but there might be better other bugs that are a bit easier to get traction on.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[kumar303]

Thanks for everyone's interest. As Matt mentioned, this is not a patch we are able to accept at this time. I have removed the contrib label for now but perhaps in Q4 2017 we can put it back.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

[willdurand]

I am going to close this issue because it is hard and not fun, so not really nice for contributors. I suggest we continue to adopt async/await for new code or when we have to touch old code that relies on Promise instead.