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

"this" binding is lost in subscribe() handlers #150

Open
samvayn opened this issue Apr 13, 2016 · 3 comments
Open

"this" binding is lost in subscribe() handlers #150

samvayn opened this issue Apr 13, 2016 · 3 comments

Comments

@samvayn
Copy link

samvayn commented Apr 13, 2016

This loop in postal.subscribe() is not dealing correctly with "this" binding and it is set to "window" in a callbacks, which results in this.cache being undefined and exception thrown.

    // Next, add the SubscriptionDefinition to any relevant existing cache(s)
    _.each( _.keys( this.cache ), function( cacheKey ) {
        if ( cacheKey.substr( 0, channelLen ) === subDef.channel ) {
            getCacher(
                cacheKey.split( _config.cacheKeyDelimiter )[1],
                this.cache,
                cacheKey )( subDef );
        }
    }, this ); 

While I see numerous instances of saving "this" into "self" in other functions - subscribe() does not do it.

@DylanTusler
Copy link

They appear to have made a stab at fixing this in 1.0.10, but the fix has broken postal for us, resulting in duplicate subscriptions being invoked for a single publish. The fix they tried was to simply remove "this" from the call, but I suspect your idea of using "self" instead would be better.

@ifandelse
Copy link
Contributor

@DylanTusler any chance you have an example of this causing duplicate subscription invocations? This isn't something I've run into yet, please let me know if it's happening and if you have an example case reproducing it, I'd love to take a look. thanks!

@DylanTusler
Copy link

Sorry, I'm not with that company any more. It was so long ago that I'm a bit shaky on what we did to even work around it. I think we stuck with a version prior to 1.0.10.

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

No branches or pull requests

3 participants