Skip to content
This repository was archived by the owner on Mar 17, 2025. It is now read-only.

Conversation

@jwngr
Copy link

@jwngr jwngr commented Nov 17, 2014

@katowulf - A few small fixes for $firebaseAuth

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should be doing any internal error handling here, perhaps triggering events in the Angular scope. Hrm. Something to think about.

Copy link
Author

Choose a reason for hiding this comment

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

Their callback will already receive the errors and they can do what they want with them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Callbacks aren't terribly convenient in all architectures. An event broadcasting strategy can be pretty handy for things like this. It's not impossible or particularly hard to add a .run() method that implements this, if one is versed in Angular and AngularFire, but it's worth pondering as part of the API as lots of people utilize $emit and $broadcast as a basic pattern (particularly when talking to directives)

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha, add it to our discussion list for the meeting this week. Sounds like a reasonable idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #474 for an alternative idea.

katowulf added a commit that referenced this pull request Nov 17, 2014
@katowulf katowulf merged commit db28d45 into master Nov 17, 2014
@katowulf katowulf deleted the jw-auth-fixes branch November 17, 2014 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants