Breaking Change: Remove SyncTasks from reactxp core and prefer ES6 promises

[berickson1]

Usage of ES6 promises is more standardized, using SyncTasks by default doesn't provide many consumers a large benefit. If they need SyncTasks promises, they can wrap in SyncTasks.fromThenable at the use site

[berickson1]

@erictraut @deregtd, looking for feedback here. ES6 promises are commonplace now. The custom error behaviour that we build into SyncTasks isn't overly valuable to consumers if they expect ES6 behaviour. We're also just wrapping ES6 promises in all the React Native callbacks.
A 2.X release would be a good time to drop this dependency in reactxp core

[deregtd]

I'm fine with this. It's time to accept that people like swallowing errors in their code and deal with it.

We still need to figure out what to do about simplerestclients, whether we add optional cancellation tokens or whatnot.

[mikehardy]

Is it worth altering the TodoList sample as well (it uses SyncTasks in ServiceManager as it starts services)? https://github.com/microsoft/reactxp/blob/master/samples/TodoList/src/ts/services/ServiceManager.ts#L55

I ask because I built my app on that sample (:pray: thank you guys) and it has served me well so far, and might be the basis for future people

[berickson1]

@mikehardy - you're right. All the extensions would need to be updated too. I was just trying to make a single surgical change as an example, especially if others weren't onboard with this change :)

[erictraut]

This is a good idea, Brent. Let’s push this through the entire source base.

[berickson1]

I left the usage of SyncTasks in ImageList sample and TodoSample because the first relies on GenericRestClient (which still uses SyncTasks) and the latter relies on NoSqlProvider (same thing)

Note: RXPTest will fail to compile unless the latest dist of reactxp is coped in (due to changed typings)

[deregtd]

@berickson1 This looks ready to merge. Any complaints?

[mikehardy]

Selfishly - my project is in between releases now so this would be a low-pressure time for me to integrate the breaking change. So I'd love to see an rc.2 with this in it.

With this change and maybe more breaking changes it might be a help to developers to mention in the release notes that focused breaking changes are happening, and are contemplated for the rc series so integrators should take care

[mikehardy]

@berickson1 I was just looking to see if I could purge SyncTasks completely from my project and I am stuck on the NoSqlProvider. It appears to me that NoSqlProvider will need them forever based on this wording?

As part of this work, we had to develop SyncTasks as a complete promise library that spurns the A+ convention of deferring all resolutions/rejections to the event loop. This is because IndexedDB's transactional semantics are such that if control is ever returned to the event loop without a pending query, the transaction is automatically closed. As such, if you defer resolution of a query, then the transaction closes before you can issue followup queries against it. To avoid this, we required synchronous resolution of promises, and SyncTasks gives us the ability to chain commands on a single transaction. Given that IndexedDB was the most limiting transactional scenario that we needed to support, we followed that pattern with all of the other database providers, and designed them such that returning control to the event loop without issuing followup queries automatically resolves the transaction.

For GenericRestClient is the goal to eliminate SyncTasks where ever not needed (thus a PR there would be interesting) or will it sit as well?

[deregtd]

An internal team here is slowly working through removing synctasks from nosqlprovider. Basically, all modern browsers have fixed that limitation that caused us to originally need to use it, so it's not needed for IE11+ and like every other vaguely modern browser.

GenericRestClient uses synctasks for cancellation. We'll need to figure out a new way to do cancellation on there for us to be able to eliminate synctasks, so don't worry about that for now.

[mikehardy]

Oh great - I'll spend no time in this area then but now I know the direction, thanks for the speedy reply.