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

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

Merged
merged 3 commits into from
Feb 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions packages/accounts-base/accounts_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export class AccountsClient extends AccountsCommon {
constructor(options) {
super(options);

this._loggingIn = false;
this._loggingInDeps = new Tracker.Dependency;
this._loggingIn = new ReactiveVar(false);
this._loggingOut = new ReactiveVar(false);

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use ReactiveVar:

this._loggingOut = new ReactiveVar(false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point

this._loginServicesHandle =
this.connection.subscribe("meteor.loginServiceConfiguration");
Expand Down Expand Up @@ -43,19 +43,23 @@ export class AccountsClient extends AccountsCommon {
// also uses it to make loggingIn() be true during the beginPasswordExchange
// method call too.
_setLoggingIn(x) {
if (this._loggingIn !== x) {
this._loggingIn = x;
this._loggingInDeps.changed();
}
this._loggingIn.set(x);
}

/**
* @summary True if a login method (such as `Meteor.loginWithPassword`, `Meteor.loginWithFacebook`, or `Accounts.createUser`) is currently in progress. A reactive data source.
* @locus Client
*/
loggingIn() {
this._loggingInDeps.depend();
return this._loggingIn;
return this._loggingIn.get();
}

/**
* @summary True if a logout method (such as `Meteor.logout`) is currently in progress. A reactive data source.
* @locus Client
*/
loggingOut() {
return this._loggingOut.get();
}

/**
Expand All @@ -65,9 +69,11 @@ export class AccountsClient extends AccountsCommon {
*/
logout(callback) {
var self = this;
self._loggingOut.set(true);
self.connection.apply('logout', [], {
wait: true
}, function (error, result) {
self._loggingOut.set(false);
if (error) {
callback && callback(error);
} else {
Expand Down Expand Up @@ -138,6 +144,15 @@ Meteor.loggingIn = function () {
return Accounts.loggingIn();
};

/**
* @summary True if a logout method (such as `Meteor.logout`) is currently in progress. A reactive data source.
* @locus Client
* @importFromPackage meteor
*/
Meteor.loggingOut = function () {
return Accounts.loggingOut();
};

///
/// LOGIN METHODS
///
Expand Down Expand Up @@ -438,4 +453,24 @@ if (Package.blaze) {
Package.blaze.Blaze.Template.registerHelper('loggingIn', function () {
return Meteor.loggingIn();
});

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return Meteor.loggingOut();
});

/**
* @global
* @name loggingInOrOut
* @isHelper true
* @summary Calls [Meteor.loggingIn()](#meteor_loggingin) or [Meteor.loggingOut()](#meteor_loggingout).
*/
Package.blaze.Blaze.Template.registerHelper('loggingInOrOut', function () {
return (Meteor.loggingIn() || Meteor.loggingOut());
});
}
73 changes: 73 additions & 0 deletions packages/accounts-base/accounts_client_tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
const username = 'jsmith';
const password = 'password';

const logoutAndCreateUser = (test, done, nextTests) => {
Meteor.logout(() => {
// Make sure we're logged out to start with
test.isFalse(Meteor.user());

// Setup a new test user
Accounts.createUser({ username, password }, () => {
// Handle next tests
nextTests(test, done);
});
});
};

const removeTestUser = (done) => {
Meteor.call('removeAccountsTestUser', username, () => {
done();
});
};

Tinytest.addAsync(
'accounts - Meteor.loggingIn() is true right after a login call',
(test, done) => {
logoutAndCreateUser(test, done, () => {
// Login then immediately verify loggingIn is true
Meteor.loginWithPassword(username, password);
test.isTrue(Meteor.loggingIn());
removeTestUser(done);
});
}
);

Tinytest.addAsync(
'accounts - Meteor.loggingIn() is false after login has completed',
(test, done) => {
logoutAndCreateUser(test, done, () => {
// Login then verify loggingIn is false after login has completed
Meteor.loginWithPassword(username, password, () => {
test.isTrue(Meteor.user());
test.isFalse(Meteor.loggingIn());
removeTestUser(done);
});
});
}
);

Tinytest.addAsync(
'accounts - Meteor.loggingOut() is true right after a logout call',
(test, done) => {
logoutAndCreateUser(test, done, () => {
// Logout then immediately verify loggingOut is true
Meteor.logout();
test.isTrue(Meteor.loggingOut());
removeTestUser(done);
});
}
);

Tinytest.addAsync(
'accounts - Meteor.loggingOut() is false after logout has completed',
(test, done) => {
logoutAndCreateUser(test, done, () => {
// Logout then verify loggingOut is false after logout has completed
Meteor.logout((error) => {
test.isFalse(Meteor.user());
test.isFalse(Meteor.loggingOut());
removeTestUser(done);
});
});
}
);
5 changes: 5 additions & 0 deletions packages/accounts-base/accounts_tests_setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Meteor.methods({
removeAccountsTestUser(username) {
Meteor.users.remove({ username });
},
});
1 change: 1 addition & 0 deletions packages/accounts-base/client_tests.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
import "./accounts_url_tests.js";
import "./accounts_reconnect_tests.js";
import "./accounts_client_tests.js";
2 changes: 2 additions & 0 deletions packages/accounts-base/package.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Package.onUse(function (api) {
api.use('random', ['client', 'server']);
api.use('ejson', 'server');
api.use('callback-hook', ['client', 'server']);
api.use('reactive-var', 'client');

// use unordered to work around a circular dependency
// (service-configuration needs Accounts.connection)
Expand Down Expand Up @@ -62,6 +63,7 @@ Package.onTest(function (api) {
'accounts-password'
]);

api.addFiles('accounts_tests_setup.js', 'server');
api.mainModule('server_tests.js', 'server');
api.mainModule('client_tests.js', 'client');
});
4 changes: 2 additions & 2 deletions packages/accounts-ui-unstyled/login_buttons.html
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<template name="loginButtons">
<div id="login-buttons" class="login-buttons-dropdown-align-{{align}}">
{{#if currentUser}}
{{#if loggingIn}}
{{! We aren't actually logged in yet; we're just setting Meteor.userId
{{#if loggingInOrOut}}
{{! We aren't actually logged in/out yet; we're just setting Meteor.userId
optimistically during an at-startup login-with-token. We expose this
state so other UIs can treat it specially, but we'll just treat it
as logged out. }}
Expand Down