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

Add Accounts.onLogout(func) Hook #6889

Closed
wants to merge 7 commits into from
Closed

Add Accounts.onLogout(func) Hook #6889

wants to merge 7 commits into from

Conversation

pmoons
Copy link
Contributor

@pmoons pmoons commented Apr 24, 2016

Accounts.onLogout(func) is practically identical to the implementation of Accounts.onLogin(func). This PR is an effort to maintain symmetry between the available Login / Logout hooks.

Use cases include destroying active session variables, reverting UI elements, and any other cleanup that needs to occur immediately after the user is logged out.

This PR is related to the following discussions:
#5984
#3867

#1074
#426

Corresponding user-facing documentation has also been provided in this PR, as well as appropriate tests.

@pmoons
Copy link
Contributor Author

pmoons commented Apr 24, 2016

Test failed in livedata package. Could someone help me discover the issue on this please? Only did work in accounts-base package.

Failed Test:

tinytest - livedata stub - reconnect non-idempotent method : !!!!!!!!! FAIL !!!!!!!!!!!
{"groupPath":["tinytest","livedata stub"],"test":"reconnect non-idempotent method","events":[{"sequence":2489,"type":"fail","details":{"type":"true","not":false},"cookie":{"name":"livedata stub - reconnect non-idempotent method","offset":0,"groupPath":["tinytest","livedata stub"],"shortName":"reconnect non-idempotent method"}}]}

EDIT It seems that checking out commit 7012957a still has the same failing test. My guess is that this issue is not related and outside the scope of this PR.

@jremi
Copy link

jremi commented May 7, 2016

This feature is very important. Thank you for working on this, hopefully this onLogout hook will be available soon.

@tmeasday
Copy link
Contributor

Can you rebase this to be off of current devel? I think this branch is off an earlier commit with failing self tests. It should be as easy as (assuming you have a remote called upstream pointing here):

# optionally
git remote add upstream https://github.com/meteor/meteor

git fetch upstream
git rebase upstream/devel
git push --force

Also, note that the docs have moved to https://github.com/meteor/docs. Sorry this PR got caught up in that change, so for now just remove the docs changes from the PR and we'll open a PR there once this commit is in.

@tmeasday tmeasday self-assigned this May 10, 2016
@jdmswong
Copy link

add this!!

@tmeasday
Copy link
Contributor

@pmoons?

@Ben305
Copy link

Ben305 commented Jun 7, 2016

This would be great 👍

@@ -88,6 +96,14 @@ Accounts.onLoginFailure(function (attempt) {
}
});

var capturedLogouts = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be an array ([])?

@tmeasday tmeasday mentioned this pull request Jun 7, 2016
4 tasks
@tmeasday
Copy link
Contributor

tmeasday commented Jun 7, 2016

Ok, I rebased and squashed some commits (and addressed @benjamn's comment) here: #7187

@convexset
Copy link

convexset commented Aug 16, 2016

Seems to be a major asymmetry here. Accounts.onLogin callbacks are called with an object argument with "signature" {type, allowed, methodName, methodArguments, user, connection}, and Accounts.onLogout callbacks are called with no arguments, and with no special function invocation context (i.e.: this)...

The relevant connection id has to be scrounged up with DDP._CurrentInvocation.getOrNullIfOutsideFiber(), which strikes me as... excessive. Whyyyyyyyyyyyyy?

At the very least, the userId of the user logging out, and current connection should be provided....

@tmeasday
Copy link
Contributor

@convexset #7433 should be released in 1.4.1 soon.

@interfaith
Copy link

is onLogout available ?
after logging in with Twitter
a way to clean up before logging out is VITAL !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants