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

Reusable Accounts constructors #4233

Closed
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
2 participants
@benjamn
Member

benjamn commented Apr 17, 2015

This pull requests reduces the assumption of a singleton Accounts namespace in the accounts-base core package, making it much easier to manage multiple accounts connections without duplicating code.

At a high level, this refactoring was all about replacing the ad-hoc Accounts object with instances of the new AccountsClient and AccountsServer classes, which inherit from AccountsCommon so that methods can be shared (yay isomorphism!).

As an example of a luxury that had to be given up in order to support multiple accounts instances, initialization code that used to run at the top level (like the auto-login code) is now called from the constructor functions.

I think it's now much clearer which methods and properties are available on the client, on the server, or in both places, since each environment has a distinct .prototype where available methods are defined.

this._initLocalStorage();
};

var Ap = AccountsClient.prototype =

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

This inheritance dance will become much cleaner when we enable ES6 class syntax in core packages.

@@ -235,11 +246,18 @@ makeClientLoggedIn = function(userId, token, tokenExpires) {
* @param {Function} [callback] Optional callback. Called with no arguments on success, or with a single `Error` argument on failure.
*/
Meteor.logout = function (callback) {
Accounts.connection.apply('logout', [], {wait: true}, function(error, result) {
return Accounts.logout(callback);
};

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

Here's a good example of a general principle I tried to follow: wherever there was a Meteor.* method that was implemented in terms of Accounts, I moved as much logic as possible into a method of AccountsClient or AccountsServer, and simplified the Meteor.method accordingly.

Changing the Meteor.* API is out of scope for this pull request, but at least we can reduce the difficulty of reimplementing what it does by making it a thinner wrapper around the AccountsClient and AccountsServer APIs.

@@ -251,6 +269,12 @@ Meteor.logout = function (callback) {
* @param {Function} [callback] Optional callback. Called with no arguments on success, or with a single `Error` argument on failure.
*/
Meteor.logoutOtherClients = function (callback) {
return Accounts.logoutOtherClients(callback);
};

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

Here's an even better example of that principle.

if (typeof __meteor_runtime_config__ !== "undefined" &&
__meteor_runtime_config__.ACCOUNTS_CONNECTION_URL) {
if (url) {
this.connection = DDP.connect(url);

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

If you pass an explicit ddpUrl to the constructor, it takes precedence here.

defaultValidateNewUserHook
];

this._deleteSavedTokensForAllUsersOnStartup();

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

Here's an example of some code that used to run at the top level getting called from a constructor function, so that it will run for each accounts instance that might be created.

secret: OAuthEncryption.seal(config.secret)
} }
);
ServiceConfiguration.configurations.find({

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

These changes don't alter the behavior of this code; they're just fixing the crazy formatting.

_.each(user.services, function (serviceData) {
pinEncryptedFieldsToUser(serviceData, user._id);
});
}

var fullUser;
if (onCreateUserHook) {

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

This should be this.onCreateUserHook. Will fix.

var self = this;

// Login handlers should really also check whatever field they look at in
// options, but we don't enforce it.
check(options, Object);

var result = runLoginHandlers(self, options);
var result = Accounts._runLoginHandlers(self, options);

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

It's a shame that we have no access to a specific AccountsServer object in methods, so we have to continue assuming the default Accounts object.

* @namespace Accounts
* @summary The namespace for all client-side accounts-related methods.
*/
Accounts = new AccountsClient();

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

It's important to define the new Accounts instance after all the code setting up its .prototype has finished running, otherwise we might call methods from the constructor that haven't been defined yet.

if (this.connection !== Meteor.connection) {
// If this.connection is not simply Meteor.connection, append a
// deterministic connection-specific suffix to the key used with
// Meteor._localStorage.

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

Using different keys with Meteor._localStorage depending on the URL of the DDP connection is really important! So is using the same keys as before when self.connection === Meteor.connection (so that folks won't have to log in again when we release these changes).


// All of the special hash URLs we support for accounts interactions
var accountsPaths = ["reset-password", "verify-email", "enroll-account"];

var savedHash = window.location.hash;
window.location.hash = "";

This comment has been minimized.

@benjamn

benjamn Apr 17, 2015

Member

Saving the hash here means it no longer matters which AccountsClient instance calls _attemptToMatchHash first.

@benjamn benjamn force-pushed the reusable-accounts-constructors branch from 433df8f to a608173 Apr 20, 2015

@benjamn

This comment has been minimized.

Member

benjamn commented Apr 20, 2015

There's an argument in favor of making AccountsClient available on both client and server, since server code might need to act as a client to an accounts server, too. I don't need that functionality yet, but it's something to think about.

benjamn added some commits Apr 16, 2015

Implement a reusable AccountsCommon constructor.
This involves moving Accounts.* methods defined in accounts_common.js onto
AccountsCommon.prototype.*.
Implement a reusable AccountsServer constructor.
Note that this constructor inherits from the AccountsCommon constructor.
Naturally both of these constructor functions should become classes once
we have support for ES6 classes.
Implement a reusable AccountsClient constructor.
There's an argument in favor of making AccountsClient available on both
client and server, since server code might need to act as a client to an
accounts server, too. I don't need that functionality yet, but it's
something to think about.
Support multiple AccountsClient instances in url_client.js.
Also share urls.* methods between all AccountsServer instances.

@benjamn benjamn force-pushed the reusable-accounts-constructors branch from aaa4309 to a2d588d May 12, 2015

@benjamn

This comment has been minimized.

Member

benjamn commented May 14, 2015

Closed via ff5fb16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment