User logging in or out overwrites onReconnect and code using onReconnect breaks resume-on-reconnect

[brettle]

_1 Upvote_ The docs say that connection.onReconnect can be used to set "a function to be called as the first step of reconnecting." However, if the user logs in or out using the connection, the accounts system will overwrite any onReconnect function that has been set. This overwriting occurs here and here. There is also the related issue that other code setting connection.onReconnect will cause the resume-on-reconnect functionality to stop working.

If you really need a reproduction repo, I can put one together, but I'd prefer to just include appropriate tests in a PR which fixes the problem.

Here's the fix I propose:

1. Deprecate connection.onReconnect.
2. Add a connection.onReset(callback) method that registers callback as one of potentially several callbacks to be called when the connection is reset/reconnected (in addition to calling connection.onReconnect(). Like onLogin, etc. connection.onReset(callback) would return an object with a stop() method that could be used to unregister the callback.
3. Change AccountsClient.callLoginMethod to call onReset(callback) instead of setting onReconnect, so that other code can't break the resume-on-reconnect functionality.

Would a PR along these lines be considered?

[avital]

Hey @brettle, thanks for the clearly explained issue.

You're right -- we document the onReconnect API even though for all practical purposes it's not useable (unless it's a connection that never logs in I suppose).

I'm curious to know what your use-case is. Can you elaborate on that?

If I were to make this change I wouldn't even keep onReconnect around since as you pointed out it couldn't really have been used anyways. I'd make it onReconnect(callback). And that would need to return a handle that has an .unregister() method that so that the accounts system can unregister the original handle when there's a login or logout on the connection.

[brettle]

Hi @avital. I have 2 specific use cases at the moment.

Use case 1: I'm working with @splendido and @zimme on a new accounts system that we hope will eventually be integrated into core. The basic idea of the new system is that it will separate creation of an identity from the creation of an account. An identity is conceptually similar to the current concept of a service account. You might have multiple Google identities, and a password identity and they could all be used to access the same account. Anyway, I would like to be able to support using the existing login services (of which there are many) to create identities instead of accounts. To do that, the client would put the connection in create-identity-not-account mode (via a method call) and then call the desired Meteor.loginWithWhatever method. On the server, a validateNewUser handler would (1) detect that the connection was in that state, (2) turn the created account into an identity (which is not stored in Meteor.users), (3) associate the identity with the connection for later retrieval by the client, and (4) deny the login. To make this scheme work, the create-identity-not-account mode of the connection needs to be re-established on reconnect so that everything works properly if the connection is lost before Meteor.loginWithWhatever gets around to calling its server login method.

Use case 2: One of the issues with the current account system is that loginWith<ExternalService> will create a new account if one does not exist. If the user is trying to sign-up, that's fine. But if the user is just trying to sign-in and forgot which service they used when they signed up, the user ends up with 2 accounts (and a sense of confusion). A good accounts UI can/should know whether the user is trying to sign-in or trying sign-up, but that needs to be communicated to the server so that it can (via a validateNewUser handler, for example) control whether an account is created. To do that, I'd like to be able to put the connection in do-not-create-a-new-account mode. Similar to use case 1, this mode needs to be maintained across reconnects.

Regarding onReconnect vs onReset, I'm fine with either but I proposed onReset because changing onReconnect would not be back-compatible, and there are at least a handful of tests in other core packages that currently use onReconnect. I don't know if it is being used by 3rd-party packages at all, but I figured better safe than sorry. Maybe onReset for 1.3 and onReconnect for 2.0 to be semver friendly?

Regarding .unregister(), I was going to use the core callback-hook package which will return a handle that has a .stop() method. That would also be consistent with onLogin, onLoginFailure, validateLoginAttempt, and validateNewUser.

Let me know if you have any other questions and whether my plan sounds reasonable. I'd like to start work on the PR soon.

[avital]

Thanks for explaining this.

Honestly how you describe the design for these new packages sounds a little contorted to me, but maybe that's the best option given our current accounts system.

Thanks for reminding me about the callback-hooks package, I totally forgot we had that. Definitely better to follow that convention.

I'm not sure we should care too much about backwards compatibility. If 3rd party packages used onReconnect up to now those would break when a user logs in, right? If you agree then there's basically no reason to support the old API which basically couldn't ever really work other than for connections that never log in.

Meteor releases aren't semver. We definitely break APIs between Meteor releases. But we should aim to keep package versions semver-compliant.

[brettle]

The implementations I described are definitely contorted but yes that is due to the limitations of the current accounts system. This is all under the covers though. The API for these packages will (hopefully) be pretty easy to understand.

Yes, 3rd party packages which currently use onReconnect would break when a user logs in, but it is conceivable that a package/app would not use the Meteor accounts system at all. Maybe it just uses DDP or has it's own accounts system. Note that if we redefine onReconnect, then when such a package/app sets onReconnect, it will break the ability to register onReconnect callbacks.

FWIW, here's a hackish way to achieve backward compatibility: Have onReconnect(callback) register the callback, and onReconnect(/* no args */) call all registered callbacks. On a reconnect, we would call onReconnect(/* no args */) like we do now. That way if no code sets onReconnect, then all callbacks registered with onReconnect(callback) get called, and if any code sets onReconnect = func, func will get called but callbacks registered with onReconnect(callback) will not. Without something like this, the ddp-client package version would presumably need to be bumped from 1.2.1 to 2.0.0 to be semver compliant, right?

The more I think about this, the more I think it would be better to just put the new functionality under a name other than onReconnect. I don't like the idea that an existing package/app that is using onReconnect as currently documented would break the new functionality by setting onReconnect. Even if such a package/app is itself broken by the current accounts system, reconnects are rare enough that such breakage might have gone undetected and as a result the onReconnect = func code might still be present and cause problems. If you don't like connection.onReset(callback), another option would be DDP.onReconnect(callback) where callback would take the connection object as an argument.

[zimme]

@avital, we're currently working on these document to try and describe the behaviours and api we expect and want.
https://github.com/meteor-useraccounts/meteor/blob/accounts-new/packages/identity/Stories.md
https://github.com/meteor-useraccounts/meteor/blob/accounts-new/packages/identity/API.md
Any feedback would be great.

Also, how high would the likelihood be that a new accounts system would be accepted into core that have almost no backwards compatibility? The reason I ask is because during our talks around a new accounts system I've felt several times that it maybe would just be better to make a new accounts system
that works as we want and need and then see if and where we can have backwards compatibility.

edit: feel free to remove if you think it's too off topic.

[brettle]

@avital, the PR I just added takes the DDP.onReconnect approach. Let me know if you'd prefer something else or need any changes. Thanks!

[brettle]

I just noticed that on the server-side there is Meteor.onConnection(callback). Perhaps instead of DDP.onReconnect(callback) we should go with DDP.onReconnection(callback) or connection.onReconnection(callback)? The latter would allow us to tie the callback to the connection object (like connection.onReconnect = callback currently does), while avoiding conflicts with existing code and using a name that is more appropriate than onReset.

[AlbinoGeek]

Just my 2 cents, but if any package is using onReconnect=func syntax right now, it actually breaks Meteor@1.2+ , the package does not work. (Can someone show me a package that does use onReconnect and still works after Meteor 1.2?)

Therein deprecating it or removing it entirely isn't a bad idea (as how it was documented, it never worked to begin with.)

[brettle]

@AlbinoGeek, I think it is possible for a package to use the onReconnect=func syntax right now without breaking Meteor@1.2+, but only if the package ensures that the new func calls the previous value of onReconnect. The meteorhacks:sikka package seems to take such an approach.

If you meant that Meteor 1.2 would break the package instead of the other way around, the answer is yes, but only if the package/app uses the Meteor accounts system. For example, it looks like the vertretungsplan-handrup app uses ddp but not accounts-base and uses the onReconnect=func syntax.

FWIW, I found the above 2 examples with this github search. It turns up a bunch of false positives, but it seems unlikely that these are the only examples.

Also, this issue appears to have been around since way before Meteor 1.2. For example the accounts system seems to have had this same issue since at least November 2012.

Anyway, I agree that the current syntax should be deprecated. The question is whether the new syntax should be connection.onReconnect(callback) or something else. I think it should be something else, not primarily so that code that uses the current syntax continues to work, but instead to ensure that code that uses the current syntax doesn't break code that uses the new syntax.

[AlbinoGeek]

@brettle Thanks for the explanation, and the link to github search; I completely forgot that was a thing.

I agree with you on the ".. use a different syntax for the new functionality .." part, as it would avoid breaking any code that used the previous method (and we should print a deprecation warning if they attempt to use the old functionality, else they would silently fail and package authors and users alike wouldn't know why.)

For example, with the new syntax; if I was a package author (or used a package) that implemented the onReconnect = func , I wouldn't realize that my functionality wasn't working (that my function wasn't being called), it would be a silent break (hence the need for a deprecation warning, or some way of telling the package user/author that the functionality has changed.)

[brettle]

FYI, the PR I created for this bug is PR #5677. I thought mentioning the issue number in the commit message would make github link to it here, but I guess not.

[zimme]

@brettle it links when it's merged and in the commit history of the repo =)

[lorensr]

+1 for changing it to onReconnect(callback).

As an author of a package that uses Meteor.connection.onReconnect, I feel like I need a disclaimer in my README saying "If you redefine onReconnect, make sure to save and call the old value, or this package and probably other stuff will break".

Also, I can confirm that the user logging in or out breaks my package by overwriting Meteor.connection.onReconnect and not saving and calling the old value.

meteor/packages/accounts-base/accounts_client.js

Line 223 in 2ccb746

self.connection.onReconnect = function () {

[vblagomir]

+1
I need this to trigger a method to register a connection.. but after user login it is getting overwritten, so not usable..

[vblagomir]

One can overcome the issue with observing changes in Meteor.status()

// on client
Meteor.autorun(function(){
  if (Meteor.status().connected) {
    console.log('test');
  }
});

this will always trigger on every reconnect

[hwillson]

Hi all - just blowing this dust off this one a bit. I know it's been a while, but I think there is definitely still value in getting these changes in (especially since onReconnect is still kinda broken). @brettle - I'm going to re-open PR #5677. If you're still around and are interested in working on it further, let me know. Otherwise I'll jump in and get it wrapped up. Thanks!