adamdbradley
commented
8 years ago Absolutely, I'm glad it's been pointed out. The first go at the conference app's data service was more about just getting a working app so we could test visual design and performance, and wasn't so much focused on the data itself (and yes I'll admit it was entirely me that whipped the service together without putting too much thought into it).
Now's a great time to reevaluate everything about how the data is being managed within the app, to include authentication and showing/hiding various elements/menus. I think it's a great idea to use observables over promises here. We'll start reviewing your version, thanks!
/cc @mlynch @jthoms1 @robwormald
ddellamico
jgw96
ionitron-bot[bot]
Short description of the problem:
As the conference app is an important resource for newcomers to the ionic ecosystem, I see vestiges of its code in much application code posted to the support forums. To the untrained eye, it looks like it was attempted to bolt the Angular2 use of Observables onto the Angular1 use of Promises, especially as pertaining to the Http client. Perhaps partially as a result of this, I believe it perpetuates a couple of idioms that are generally considered antipatterns, specifically the explicit promise constructor (which could be avoided by using Observable.toPromise) and the ghost promise (which could be avoided by a unilateral Promise.resolve). However, I think it would be more idiomatic to Angular2 in general to avoid using Promises at all here. To at least get the conversation going, and to avoid simply criticizing without attempting to provide some sort of constructive suggestion, I would welcome comments on this version of conference-data.ts. There are a handful of other small changes to other files necessitated; all are made in that forked branch, and at least in my environment, that version builds without error and seems to function as intended.