awulder / angular-prismicio

AngularJS service for prismic.io
MIT License
32 stars 15 forks source link

Exposing api and ctx in order to make advanced queries #13

Closed rudyrigot closed 10 years ago

rudyrigot commented 10 years ago

This would solve #10 and #11, but I'm fairly new to Angular JS, so let me know if you feel the way I've done it is relevant.

Also, I have left the non-promise form of submit() that is in the kit, but we can change that. An idea how: extending the Api class into an ApiWithPromise class, and extending the SearchForm class into a SearchFormWithPromise class. The only differences are: ApiWithPromise.form(formName) returns a SearchFormWithPromise instead of just a SearchForm, and SearchFormWithPromise.submit() returns a promise instead of taking a callback. And of course, Prismic.api() would return an ApiWithPromise object. Not sure it's the best way to make it happen; we could also monkey-patch stuff but I'm not sure it would be as clean. Not even sure it's relevant to want to make that callback into a promise. Let me know what you think!

rudyrigot commented 10 years ago

And with ctx exposed, here's the commit to fix the issue I had on the starter project I made yesterday: https://github.com/rudyrigot/javascript-angular-starter/commit/43a85ddf815bee315eac537df9b7ba5e13ee1f6f

(Right now, the bower.json is pointing at my GitHub branch to make it work)

dlecan commented 10 years ago

@rudyrigot you would like to update Prismic JS kit to return promises instead of using callbacks ? What kind of promises ? ES6 promises ? You're launching an Angular + Prismic development, we need IE8 compatibility :)

rudyrigot commented 10 years ago

Older IE compatibility is underway (see here and here).

We talked about doing it with promises directly in the kit when we first built it, and we elected not to. The reasons are cultural across the JS community, and technical:

Those two reasons don't apply much to using prismic.io with Angular (I believe more people using Angular have heard of promises, and dependencies are well-organized); so although I don't think the JS kit should work with promises, I think it'd be cool if angular-prismicio did.

About this PR though, I'm more interested in feedback about how I exposed what I exposed, and how to expose it better.

dlecan commented 10 years ago

@rudyrigot In my PR, I removed most of the callbacks to have a cleaner code and better errors handling. I introduced promises maybeApi and maybeContext which can be exposed as API methods if necessary.

So yes, for me, exposing context and api can be useful and do it through promises is a good idea.

rudyrigot commented 10 years ago

Yep, I saw your PR, I hope it will be merged soon. :) But from what you're saying, I guess it would probably make sense to rebuild what I did on top of your PR when it's merged, right?

awulder commented 10 years ago

@rudyrigot Should I first merge the PR's of @dlecan?

rudyrigot commented 10 years ago

Yes, definitely; I'll adapt mine when you do, and you guys will then tell me what you think of it.

awulder commented 10 years ago

@rudyrigot I have merged the PR of @dlecan into master.

rudyrigot commented 10 years ago

Yep, there you go, I merged master into this branch, and rewrote my code to adapt to it.

awulder commented 10 years ago

Looking good, thanks.