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

Observable API and listeners fixups #593

Merged
merged 2 commits into from
Feb 7, 2016

Conversation

meejah
Copy link
Contributor

@meejah meejah commented Feb 5, 2016

This makes the ObservableMixin less fragile for use as a mix-in by eliminating its parameter-taking __init__ method, and some cleanups and docstrings. It also adds an (optional!) list of "valid events" to make users of .on() (or even internal uses of .fire()) complain if you mis-type an event name.

The other commit tackles the LTS listener API by fixing how they're fired/invoked. Doing the invokes in e.g. onJoin won't work with most of the code out there (including all the examples) because the API contract never specified to call super() (and basically no code does this). This means that the 'join' event is basically never fired properly.

So, as per LTS API discussions there are 5 events. Note that I made one subtle change that makes a big difference (to when 'join' fires):

  • join: right before we call onJoin
  • leave: right after we call onLeave
  • ready: after all 'join' listeners have completed, and after onJoin has completed
  • connect: right before onConnect
  • disconnect: right after onDisconnect

Above "completed" means the method has run, and any future/deferred returned has also completed. The reason I made 'join' fire before onJoin has run and completed is, consider code like:

class Foo(ApplicationSession):
    def onJoin(self, details):
        yield self.call(...)
        # etc
        yield self.leave()

That is, things that use onJoin as essentially a "main" function -- do all their work, and then disconnect the session. In the above, if we wait for onJoin to complete before we fire the 'join' listeners, then we very counter-intuitively would fire 'leave' before firing 'join'.

So, now 'join' fires as soon as we get the Hello message, and then the previously-discussed 'ready' fires after onJoin has completed (and all 'join' listeners have completed). Also, this is implemented such that it doesn't matter which of the over-ridable methods in ApplicationSession you choose to override, and doesn't care if you call super() or not. Also, any errors from listeners are logged (but ignored).

We fire all 5 "listener" events without depending on subclass
behavior to properly call super(), as this breaks almost all
existing code and subtly chagnes the API (e.g. most examples
just blindly override onJoin).

There are 5 events: join, leave, ready, connect and disconnect
as per LTS-api discussions.
Also add an optional list of 'valid events'
@meejah meejah force-pushed the listener-observable-api-fixup branch from c7981c5 to 5b0e697 Compare February 6, 2016 06:39
oberstet pushed a commit that referenced this pull request Feb 7, 2016
@oberstet oberstet merged commit 1e50e9f into crossbario:master Feb 7, 2016
@oberstet
Copy link
Contributor

oberstet commented Feb 7, 2016

I like that! It brings a lot of non-invasive flexibility ..

@meejah meejah deleted the listener-observable-api-fixup branch February 11, 2016 03:06
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.

2 participants