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

Give presence.subscribe callback arg the same behaviour as channel.subscribe #526

Merged
merged 2 commits into from Jul 11, 2018

Conversation

2 participants
@SimonWoolf
Copy link
Member

SimonWoolf commented Jul 11, 2018

writing a realtime presence test, was annoyed that the presence.subscribe callback arg was only called in the event of an attach error, wheras the channel.subscribe callback arg is called once attached. this makes them behave consistently

@SimonWoolf SimonWoolf requested a review from funkyboy Jul 11, 2018

if(callback) {
channel.attach(callback);
} else {
channel.autonomousAttach();

This comment has been minimized.

@paddybyers

paddybyers Jul 11, 2018

Member

Why this? (I realise this predated this PR.)

It's no different from attach except that it doesn't call the callback. Why not in attach() simply have callback = callback || noop or callback = callback || (err) => err && Logger...... ?

This comment has been minimized.

@SimonWoolf

SimonWoolf Jul 11, 2018

Member

or callback = callback || (err) => err && Logger......

When autonomousAttach was added in 0.9, its purpose was to emit an error event with a specific code, that was to be emitted only for attaches triggered autonomous or implicitly (i.e. not due to the dev calling attach()).

In a later version of the 0.9 spec, we changed our minds about the error event and got rid of it.

But autonomousAttach managed to hang on, a little sadly, a bit vestigial, just wandering around doing a bit of logging now and again, resigned to be deleted once someone took notice of its continued existence.

I'll fix.

Remove realtimechannel.autonomousAttach()
When autonomousAttach was added in 0.9, its purpose was to emit an error event
with a specific code, that was to be emitted only for attaches triggered
autonomous or implicitly (i.e. not due to the dev calling attach()).
081a178#diff-ee8e6cae22c16e2972f2b341a26d2ed0R193

In a later version of the 0.9 spec, we changed our minds about the error
event and got rid of it.

But autonomousAttach managed to hang on, a little sadly, a bit
vestigial, just pottering around doing a bit of logging now and again,
resigned to be deleted once someone took notice of its continued
existence.
@paddybyers
Copy link
Member

paddybyers left a comment

LGTM, thanks

@SimonWoolf SimonWoolf merged commit a921e38 into master Jul 11, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@SimonWoolf SimonWoolf deleted the presence-subscribe branch Jul 11, 2018

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