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

Make async/await available to the code base #43

Open
iconix opened this issue Sep 20, 2016 · 4 comments
Open

Make async/await available to the code base #43

iconix opened this issue Sep 20, 2016 · 4 comments
Assignees
Labels

Comments

@iconix
Copy link
Contributor

iconix commented Sep 20, 2016

PR #22 introduced a getCachedValue method on the Storage abstraction in frontEndGlobals.ts. It was created to avoid the asynchronicity of calling getValue inside of the ratings helper feature, which was making ratingsPanel.tsx much less unit testable.

See full discussion.

getCachedValue could be avoided if we could await the completion of some asynchronous tasks before moving on with execution in the calling method.

So this task tracks investigation and work needed to polyfill async/await. Some initial questions:

  1. Do we want to wait for ES8 (ES2017) to (maybe) include it, and for browsers to then support ES8? (caveat: we still need to continue to support IE 10+, which only supports up to ES5)
  2. Is there a reliable npm async/await polyfill module available if we don't wish to upgrade to ES8?
@iconix
Copy link
Contributor Author

iconix commented Oct 4, 2016

I think this should be addressed sooner rather than later.

During Ratings Prompt testing, I could get into a state where I would manually delete a storage value (e.g., doNotPromptRatings) using Storage Explorer, but the Clipper would ignore this because the value was already cached.

This is a pretty dev-specific scenario where I was internally messing with storage internals, but I'm afraid leaving this paradigm around could encourage more use of it by engineers and lead to bigger unexpected behaviors.

@iconix iconix self-assigned this Oct 4, 2016
@iconix
Copy link
Contributor Author

iconix commented Oct 4, 2016

While true that async/await won't be supported by native JavaScript until at least ES8 (ES2017), Typescript 1.7+ has implemented async/await on top of ES6 promises.

So here are the steps we need to take to close this issue:

  1. Upgrade TypeScript target to ES6
  2. Downtranspile ES6 to ES5 (maybe even ES3)
  3. Add an ES6 polyfill: either es6-shim (+es5-shim), or core-js (as TypeScript + core-js is the official compiler+polyfill recommendation by the official compatibility table) -- in order to continue supporting IE 11-, PhantomJS, Mithril, etc.
  4. Remove one-off ES6 polyfills (es6-promise)
  5. Lots of testing across all supported browsers
  6. Reimplement ratings prompt to use async/await instead of getCachedValue

@iconix
Copy link
Contributor Author

iconix commented Oct 5, 2016

Investigation/implementation notes:

  • Because our tsconfig.json does not specify a target, we've been targeting the default of ES3 all along, not ES5 (source)
  • PhantomJS does not support ES6 syntax, tests are broken (source)
  • Informative thread on what Typescript + core.js actually means on the Kangax compat table. Also includes additional discussion around the target vs lib TS compile flags: core-js and TypeScript - how do these fit together? microsoft/TypeScript#3956
  • ES6 provides a standard module format, so we may not need CommonJS anymore... (converter)
  • We can remove bind_polyfill.js for tests now that we're on PhantomJS 2 (source) (we know this because node-qunit-phantomjs is targeting Phantom 2.1.3)
  • New babelify gulp task to run Babel with preset es2015 is prohibitively slow w/ npm 2 (2+ minutes to run) - had to upgrade to npm 3 (27s to run- better but still slow...) (source)

Syntactical changes in ES6:

@leakymemory
Copy link
Contributor

If we can hold on just a little longer, TypeScript 2.1 will support ES5/ES3 (currently scheduled for November 2016). It's already implemented in TypeScript@next if we wanted to play around with it.

https://github.com/Microsoft/TypeScript/wiki/Roadmap

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

No branches or pull requests

3 participants