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

Protocol Promises #72

Closed
wants to merge 2 commits into from
Closed

Conversation

burn2delete
Copy link
Contributor

added promise threading macro

@@ -29,6 +29,18 @@
[matchbox.registry :refer [register-listener register-auth-listener disable-auth-listener!]]
#?(:cljs cljsjs.firebase)))

;; macros
(defmacro promise-> [& args]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach - we haven't leveraged the "new" promise return values in Matchbox yet.

Rather than creating our own (generic) promise sugar however, why not just use something specialised?

Existing callback error handling also needs work.. #40

@burn2delete
Copy link
Contributor Author

I wasn't sure how firebase would interact with a generic promise lib, regardless of being standards compatible.

My main use case is catching read attempts when unauthorized to access a location.

@burn2delete burn2delete changed the title Promise Threading Macro Protocol Promises May 16, 2016
@burn2delete
Copy link
Contributor Author

https://github.com/flyboarder/matchbox/tree/promise-thread

has my latest rendition of the protocol and promise based implementations. JS seems fully functional and tests are passing, JVM needs some love 😄

@crisptrutski
Copy link
Owner

@flyboarder would you mind opening a new PR from the branch you've been working on?

Will make it easier to grok all the changes :)

@burn2delete
Copy link
Contributor Author

@crisptrutski Since the new v3 API was released, I started on a fresh take of the API bindings here (not matchbox based):
https://github.com/flyboarder/firebase-cljs

I ran into a slight problem, many of the Firebase types are dynamic and cannot currently be extended by cljs as it does not support dynamic runtime extension [CLJS-527].

I am implementing those protocols as functions for now. The bindings are mostly complete, except for the new storage API, however they do not include the JVM API.

@crisptrutski
Copy link
Owner

That CLJS ticket seems to have just fizzled out without firm rejection, wonder what David's feeling are on the subject now given how much the project has moved since then.

Wishing you good fortune on your cljs-only adventures, perhaps matchbox will just wrap that library and provide a JVM companion in the future 😄

@burn2delete
Copy link
Contributor Author

burn2delete commented May 22, 2016

That CLJS ticket seems to have just fizzled out without firm rejection, wonder what David's feeling are on the subject now given how much the project has moved since then.

I spoke with David about it on slack, it needs to be revisited, I assume reimplementation will move things along but I am not comfortable working on the language itself.

Wishing you good fortune on your cljs-only adventures, perhaps matchbox will just wrap that library and provide a JVM companion in the future 😄

This was my intent as my attempt at implementing this in the promise branch lead to unreadable code 😸
The API is also much larger now, including cloud storage, and other services for android/jvm which do not apply to web.

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

Successfully merging this pull request may close these issues.

2 participants