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

Added in Meteor.loggingOut() and related Blaze helpers. #8271

Merged
merged 3 commits into from Feb 15, 2017

Conversation

Projects
None yet
5 participants
@hwillson
Member

hwillson commented Jan 19, 2017

Hi all - this PR is intended to help address issues #769 and #1331. It's essentially a re-working of the very old PR #826 (submitted by @raix - thanks!). I've modified it to work with the current codebase and added some tests. Let me know if you have any questions / need anything changed. Thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 19, 2017

I forgot to mention - I created a small runnable app that shows the loggingOut helper firing (along with Meteor.loggingOut()), in-case anyone wants to see these changes in action. Thanks!

@raix

This comment has been minimized.

Contributor

raix commented Jan 19, 2017

Thanks, 4 years old :)

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 19, 2017

Hehe, I know - we're in the process of cleaning up old issues @raix so this was uncovered. There is an amazing amount of good stuff, like your changes, that got overlooked! Hopefully more of it starts to filter in soon. Thanks again!

@@ -17,6 +17,9 @@ export class AccountsClient extends AccountsCommon {
this._loggingIn = false;
this._loggingInDeps = new Tracker.Dependency;

this._loggingOut = false;
this._loggingOutDeps = new Tracker.Dependency;

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

maybe use ReactiveVar:

this._loggingOut = new ReactiveVar(false)

This comment has been minimized.

@hwillson

hwillson Jan 20, 2017

Member

Thanks for the feedback @raix! I didn't use a ReactiveVar since these changes were modelled after the existing loggingIn code. That being said, using a ReactiveVar would definitely clean the code up a bit. I'll look into making these changes, and consider updating the code for loggingIn as well (and add some new tests for that part of the code since there don't seem to be any). Thanks again for taking a look at this!

This comment has been minimized.

@raix

raix Jan 20, 2017

Contributor

Good point

if (this._loggingOut !== x) {
this._loggingOut = x;
this._loggingOutDeps.changed();
}

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

this.loggingOut(x);

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

Actually just skip this helper function

*/
loggingOut() {
this._loggingOutDeps.depend();
return this._loggingOut;

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

return this._loggingOut.get();

* @summary Log the user out.
* @locus Client
* @param {Function} [callback] Optional callback. Called with no arguments on success, or with a single `Error` argument on failure.
*/
logout(callback) {
var self = this;
self._setLoggingOut(true);

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

self._loggingOut.set(true);

self.connection.apply('logout', [], {
wait: true
}, function (error, result) {
self._setLoggingOut(false);

This comment has been minimized.

@raix

raix Jan 19, 2017

Contributor

and we could add .set here too

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 22, 2017

I updated things a bit to leverage ReactiveVar's for both loggingIn and loggingOut, and added some additional tests for the loggingIn side of things. Thanks all!

@abernix

This comment has been minimized.

Member

abernix commented Jan 26, 2017

I'll try to review this a bit more in detail during/before next week's issue club (couldn't get to it yesterday) but first pass looks good. My only initial thought might be any possibility of a term aside from loggingInOut for the helper? Possibly something along the lines of authChanging, but maybe even just adding the Or in there (loggingInOrOut)?

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 26, 2017

Sounds good @abernix - it's been 4 years, so no rush! 😄 I like your loggingInOrOut suggestion more than authChanging; authChanging might be a bit too ambiguous, since auth can be taken to mean different things (like "authentication" or "authorization"). I'll adjust - thanks again!

* @isHelper true
* @summary Calls [Meteor.loggingOut()](#meteor_loggingout).
*/
Package.blaze.Blaze.Template.registerHelper('loggingOut', function () {

This comment has been minimized.

@benjamn

benjamn Feb 8, 2017

Member

I realize there is already code in this file that assumes Package.blaze is defined, but I think I would prefer if we were a bit more defensive about checking whether blaze is installed, since it's only a weak dependency of accounts-base.

This comment has been minimized.

@hwillson

hwillson Feb 8, 2017

Member

Sounds good - I'll get this resolved - thanks!

This comment has been minimized.

@hwillson

hwillson Feb 11, 2017

Member

Just reviewing this now - it looks like there is a Package.blaze check just above the helper registration code. Do you think this is defensive enough? I can expand this further by converting the Package.blaze check into a Package.hasOwnProperty('blaze') check, paired with some additional checks to make sure the blaze object contains a Blaze property, but this might be overkill (I think it's pretty safe to assume Package.blaze really only points to a valid blaze object, if truthy). Let me know though - thanks!

This comment has been minimized.

@mitar

mitar Feb 11, 2017

Collaborator

I think Package.blaze is good enough. This is what everyone is doing for weak dependencies.

@benjamn benjamn merged commit 4f3713f into meteor:devel Feb 15, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment