Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt async/await where approriate #70

Closed
kitsonk opened this issue Sep 7, 2016 · 8 comments
Closed

Adopt async/await where approriate #70

kitsonk opened this issue Sep 7, 2016 · 8 comments
Assignees
Milestone

Comments

@kitsonk
Copy link
Member

kitsonk commented Sep 7, 2016

Down emitting async/await when targeting ES5 is available in TypeScript 2.1.0-dev.20160907 or later.

We need to investigate how we can utilise this with our dojo-shim/Promise and adopt the pattern which will make many async code more readable without needing a complex chain of .then() callbacks.

@kitsonk kitsonk changed the title Adopt async/await where approriate Adopt async/await where approriate Sep 7, 2016
@kitsonk
Copy link
Member Author

kitsonk commented Sep 9, 2016

Early research appears that for the ES5 down emit, TypeScript expects Promise to be in the global namespace. So our pattern of importing our dojo-shim/Promise module will not work with async await.

We already had to do this with Symbol because the well known symbols need to be decorated on a global Symbol to make sure they are the "right ones". We may have to adopt this as well, though it breaks our design ethos of not polluting the global namespace with our shims to ensure we don't cause regressions.

Also, our Promise implementation doesn't fully offload to native when present. I wasn't familiar with the reasons that led to that as the pre-dated me, but if we are going to have to load our promise shim into global when not present, we should consider not doing so when the native is present, thereby having a native Promise behaviour. I don't know what issues that will cause/break.

Also, we considered in dojo/shim#16 adding the cancel functionality to Promise and getting rid of Task in advance of cancellable promises being ratified by TC39.

@rorticus
Copy link
Contributor

rorticus commented Nov 8, 2016

Async/Await with dojo-shim/Promise

The Problem

The problem with using TypeScript’s new async/await features is that it reserves a Promise type in the global namespace, resulting in a duplicate key error for the following example,

import { Promise } from 'dojo-shim/Promise';

async function do(): Promise<boolean> {
    return Promise.resolve(true);
}

Solutions

Don’t use Promise

The easiest solution, although possibly the least pleasant, is to not use Promise as the import name.

For example,

import { DojoPromise } from 'dojo-shim/Promise';

async function do(): DojoPromise<boolean> {
    return DojoPromise.resolve(true);
}

Note that although the Promise type is referenced in the transpiled code, it is not used (it is only used if DojoPromise is not passed in as the promise class).

Type Alias Promise

Another solution is to use a type alias to redefine dojo-shim/Promise to Promise.

import { DojoPromise } from 'dojo-shim/Promise'
type Promise<T> = DojoPromise<T>;

async function do(): Promise<boolean> {
    return Promise.resolve(true);
}

While this is a little bit closer to the transparent use of Promise, it is similar to the previous method in that the compiled code only references DojoPromise.

Global Promise Typings

A third solution consists of,

  • Make dojo-shim/Promise global.
  • Add typings for global Promise object to dojo-typings.

Usage would look like,

import 'dojo-shim/Promise';

async function do(): Promise<boolean> {
    return Promise.resolve(true);
}

Note the import of dojo-shim/Promise without assigning any of the exported properties. This is because dojo-shim/Promise now creates a Promise object in the global scope.

Of these solutions, I think this one is desirable because it requires few change to existing code.

You can see an example of this solution in the following branches,

Setting up global promise and typings,

Usage example in which some tests are rewritten to use async/await,

@kitsonk if you think this looks OK I can go ahead and create a PR for the shim/typings changes and maybe a few for typings in some packages, like core?

@kitsonk
Copy link
Member Author

kitsonk commented Nov 9, 2016

My feeling was that we would go with the last option.

I would agree that we should have a separate set of unit tests for async/await with our promise implementation (outside of the tests/Promise.ts).

We also need to check that our solution would place nicely with Bluebird, potentially even including a set of tests for that, and that it is roughly "transparent" if someone uses Bluebird instead of our Promise implementation.

@rorticus
Copy link
Contributor

rorticus commented Nov 9, 2016

@kitsonk how do you see most people using Bluebird? If they just import it like this, they can use it along side of our promise solution (similar to solution 1)

import * as Blue from 'bluebird';

async function beUseful() {
    return Blue.resolve(1);
}

It looks like if you include Bluebird via a script tag however, it inserts itself into the global scope.

@kitsonk
Copy link
Member Author

kitsonk commented Nov 9, 2016

I am thinking they would use that as the global Promise solution. We would still write internal code, which needed to make sure the Promise was loaded as:

import 'dojo-shim/Promise`;

But that shouldn't interfere with someone using @types/bluebird and loading bluebird globally.

Actually I realised an issue with your first suggestion, it is the default export, which people may still want to do:

import DPromise from 'dojo-shim/Promise';

async function do(): DPromise<boolean> {
    return DPromise.resolve(true);
}

@dylans dylans added this to the 2016.11 milestone Nov 11, 2016
@dylans dylans modified the milestones: 2016.11, 2016.12 Dec 5, 2016
@rorticus
Copy link
Contributor

rorticus commented Dec 8, 2016

OK, I wrote some tests that use async/await using Bluebird (with dojo promise not loaded), Dojo promise (with bluebird not loaded), and Bluebird with dojo promise loaded. Everything seems to be working as intended :)

The code can be seen here,
dojo/shim@master...rorticus:async-await but note that this won't be useful until we're on TS 2.1.

@rorticus
Copy link
Contributor

PR is here, dojo/shim#64

@rorticus
Copy link
Contributor

PR merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants