launchpad-dev team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

On Tue, 31 Jan 2012, Ian Booth wrote:

> 
> > So no, don't do:
> > 
> >  * global event publishing
> >  * YUI.Env as a global namespace
> > 
> > This last is especially ugly.  It defeats the whole point of the js
> > sandbox when there's no need for it.
> > 
> > How do we fix these issues then?
> > 
> > Everything should live in a proper YUI module now.  Any module that
> > depends on functionality in another should be sure that dependent
> > module is included via the "use" block.  If something needs to be
> > shared across modules, then that functionality should be broken out
> > into its own module.  I'm not able to offer more than this without
> > digging deeper into the specifics of the failing code, but I am highly
> > confident that we should avoid the two suggestions above at all costs.
> > 
> > And apologies if I'm being too harsh here.  I've had a grouchy Monday,
> > and I'm just trying to make sure I'm clear that this is not a pattern
> > we want to start in Launchpad.  If we need to talk about design
> > choices or about how to best refactor the spots causing us issues,
> > then I can stay later one day this week to catch up with you, Ian,
> > about it.
> 
> Here's a specific use case where I can't see right now a solution which
> does not require global events, especially if we want to implement a
> solution with the correct IOC and minimal coupling.
> 
> In client.js we have a model cache implementation. The namespace is
> lp.client and there's an update_cache() method. When update_cache() is
> invoked, we need to let interested parties know what changes have been
> made. This is a classic publish-subscribe pattern: the publisher (the
> cache object) has no knowledge of what consumes it's update
> notifications (the subscribers being bespoke bits of code throughout the
> app). And nor should it, it's re-usable infrastructure etc etc.
> 
> A common pub-sub implementation is to use broadcast events as the
> transport. Message queues etc can also be used. But, this work was
> originally done using YUI's event system. And it worked a treat before
> we switched away from a single, shared YUI instance. So the cache simply
> calls:
> 
> Y.fire('lp:context:foobar:changed', {new/old foobardata});
> 
> It has no knowledge of who is interested in these updates. On the
> subscriber side, there exists on various pages code like:
> 
> YUI().use('io-base', 'lp.ui', 'lp.client', function(Y) {
> 
>     Y.on('lp:context:deb_version_template:changed', function(e) {
>         Y.lp.ui.update_field('#debian-version dd', e.new_value);
>     });
>     Y.on('lp:context:base_branch_link:changed', function(e) {
>         Y.lp.ui.update_field('#base-branch dd', e.new_value_html);
>     });
> });
> 
> So the page listens to model updates and does stuff when they are
> received. But the code now lives in it's own sandboxed instance, and
> events fired from the cache are no longer received.
> 
> Rick provided some sample code which worked across instances
> http://paste.mitechie.com/show/523/, but in that example, the code doing
> Y.fire had to know about the YUI module which was to receive the events.
> We must not allow that pattern to be used here. The cache must not know
> about how it is used or what uses it or be forced to YUI import the
> subscriber code via YUI.use().
> 
> At the moment, I cannot see a solution that:
> - avoids the use of global events
> AND
> - does not introduce unnecessary coupling
> - does not mess up dependencies
> - does not force lower layer infrastructure to know about bespoke app code
> - does not force us to migrate immediately away from our legacy codebase
> 
> It could be that I've looked at this too long and my brain cannot see an
> obvious solution. Now's the chance to show me how dumb I am. Go ahead,
> you know you want to.....

I gave it a quick look over and I think this diff is approx how I'd attempt
to solve the issue in a way that would work.

https://pastebin.canonical.com/59124/

The idea of global events can be pretty bad just because of potential
naming collisions and such anyway. It's the same as why we don't declare
global variables across the JS.

You're right in that there are generally two ways to do events. You either
pull in an object you want to watch and manually bind onto it. This is true
even in Python. For instance, in SqlAlchemy, when I want to monitor an ORM
model for events, I import that ORM model, then bind to its changed event.

The subscriber knows who he's interested in, and while the publisher
doesn't care or have to know, the subscriber is able to import/find the
thing it's interested in and bind.

In this case, the subscriber is binding to an event lp.context. So it knows
it wants to know about things in the lp.context space. So what I did was to
create a lp.context.event module in YUI. lp.client also now requires it. In
this way, lp.client knows that it wants to publish events through the
lp.context namespace, however it knows nothing about how is on the other
end.

The subscriber then also knows it cares about events in the lp.context
namespace. It also requires that new small module and will use it to watch
for events. This module is now handling bringing the two namespaces
together so that events should fire/catch just fine. It also makes it a bit
clearer that only events in the lp.context namespace are going on here and
makes it easier to find all the publishers/subscribers in that area in case
there was a bug or it needed refactoring.

Testing is also easier since there's a shared module we can use to help
test with and hopefully we could build upon something like this to more
easily catch these issues in the future.

I'm just up and looking at the problem for a few seconds so let me know if
I'm missing something. I do see that lp.client actually fires events on
some namespace based on a "cache_name" so that would have to be taken into
account, but I just grabbed the example events you were posting above in an
effort to get this out.

-- 

Rick Harding

Launchpad Developer
https://launchpad.net/~rharding
@mitechie