Dispatches to change listeners are suppressed while another dispatcher is active

[fredkilbourn]

This is perhaps safe for the average developer, but was not documented and took me a while to figure out why i could not trigger another change event inside of an event handler for a change on the same collection. There are certainly use cases for this, and I'm submitting a pull request that adds a flag to override this behavior.

[Stuk]

Would you mind describing the use case you have?

[fredkilbourn]

I have, for instance, some centralized code that is responsible for tracking and responding to state changes in various other components of my software. The centralized code tracks these various state changes in a collections.js Dictionary and I set up a map change listener on this Dictionary.

The code then, in some cases, responds to the change of a particular key by triggering some other kind of action that results in the change of a subsequent different key, and then when that happens the change listener doesn't fire on subsequent changes. This is because the execution path is still recursive to the initial Dictionary change that triggered it.

I understand that descriptor.isActive is in place to protect people from infinite loops (assuming), but this is honestly something that the user of the library should manage outside of the library. At the very least, I feel this behavior of suppressing subsequent change events should be documented (it would have saved me a lot of time debugging) and have an option to disable the behavior if you actually understand the consequences (hence my pull request).

Basic pseudocode of my problem case:

var _state = new collections.Dict(
{
    logged_in: false,
    user: null,
    accepted_terms: false,
    metadata_loaded: false
} );

_state.addMapChangeListener( function( value, key, map )
{
    switch( key )
    {
        case "user":
            _state.set( "accepted_terms", /* logic */ );
            break;
        case "accepted_terms":
            if( value === true )
            {
                //load global metadata if necessary / possible
                _state.set( "metadata_loaded", /* logic */ );
            }
            break;
        case "metadata_loaded":
            console.log( "load other resources here" );
            break;
    }
} );

[Stuk]

Thanks for explaining! We definitely need to explain this in the documentation.

I'll defer any decision on a the PR to @kriskowal

[marchant]

Fred, looks like this patterns would be more interesting/relevant/frequent with something like a Dictionary compared to an array or a set, what do you think?

On Apr 14, 2015, at 6:40 AM, Fred Kilbourn notifications@github.com wrote:

I have, for instance, some centralized code that is responsible for tracking and responding to state changes in various other components of my software. The centralized code tracks these various state changes in a collections.js Dictionary and I set up a map change listener on this Dictionary.

The code then, in some cases, responds to the change of a particular key by triggering some other kind of action that results in the change of a subsequent different key, and then when that happens the change listener doesn't fire on subsequent changes. This is because the execution path is still recursive to the initial Dictionary change that triggered it.

I understand that descriptor.isActive is in place to protect people from infinite loops (assuming), but this is honestly something that the user of the library should manage outside of the library. At the very least, I feel this behavior of suppressing subsequent change events should be documented (it would have saved me a lot of time debugging) and have an option to disable the behavior if you actually understand the consequences (hence my pull request).

Basic pseudocode of my problem case:

var _state = new collections.Dict(
{
logged_in: false,
user: null,
accepted_terms: false,
metadata_loaded: false
} );

_state.addMapChangeListener( function( value, key, map )
{
switch( key )
{
case "user":
_state.set( "accepted_terms", /* logic _/ );
break;
case "accepted_terms":
if( value === true )
{
//load global metadata if necessary / possible
state.set( "metadata_loaded", / logic */ );
}
break;
case "metadata_loaded":
console.log( "load other resources here" );
break;
}
} );
—
Reply to this email directly or view it on GitHub #121 (comment).

[fredkilbourn]

@marchant, I personally don't feel it's the responsibility of the library to predict the patterns that users of that library should be using. There well could be other cases that I haven't thought of or run across personally. If you look at custom events with the jQuery library for instance, there is no safety check for infinite loops.

If you want my honest opinion, I don't think it has a place in this library, the isActive checks should be removed and the library should do whatever the user of library tells it to do, even if that means an infinite loop. I'm obviously not the maintainer though and you guys may have your reasons for keeping this check in place. If you do feel like keeping it, it would be nice to at least add in a way to disable this check like I proposed with my pull request #122. (and have it documented)

[fredkilbourn]

@Stuk, @marchant, @kriskowal: any feedback on this? it would be nice to know if this patch will be expanded upon and implemented or if i should consider it an internal patch that i maintain on my own