Add flag to allow active descriptors in listeners #122
Conversation
There are times when listeners might want to make changes that trigger further events in a collection. I had a bug in my code around this point and only realized that further collection changes are suppressed by default. This patch adds an override flag to allow for descriptors to fire their listeners again even if currently active. Signed-off-by: Fred Kilbourn <[email protected]>
|
Assuming you see validity in my suggestion, it is probably a good idea to add this flag to property-changes.js as well, but let me know. |
Signed-off-by: Fred Kilbourn <[email protected]>
|
I have no philosophical objections to this change. When I factored observers into pop-observe, I omitted the feature entirely. If applied, it should be applied consistently across all observer classes. |
|
allowActive isn't a good make, it should be "allowsActive", but that's not really telling much, allowsNestedDispatch feels more appropriate, please update |
Updating master
- Clean up implementation, update comments - Also implement in property-changes.js - Make sure isActive and allowsNestedDispatch keys are defined respective get(Map|Property|Range)ChangeDescriptor methods Signed-off-by: Fred Kilbourn <[email protected]>
|
I've updated my branch as follows:
If this pull does get accepted, will someone be updating the collections.js docs accordingly? @kriskowal - As a relative outsider, I don't know if I fully understand the context of your comment, but if you plan on tearing out the isActive logic altogether will you just reject my pull and strip that logic out yourself? |
|
@fredkilbourn No, can’t. Backward compatibility across version 1 has to be maintained, and that’s the "latest" tag in npm. |
|
@kriskowal Okay, I get it. So how's my updated pull request looking now? |
|
@Stuk, @marchant, @kriskowal: Is there anything that needs to be fixed in this pull? AFAIK it shouldn't have any backwards compatibility issues while adding better control over the nested triggering behavior. Sorry to bump it's just been a number of months now. |
|
@fredkilbourn sorry about that, you're right to give us a poke. I no longer have write access to this project, but I would suggest adding some tests for the new functionality so that it's clear that it works, and to ensure that it doesn't get broken in the future. |
|
Can you rebase on master @fredkilbourn ? |
|
@hthetiot Wow can't believe it's been 2 years! I can fix it up but I'll have to set my environment back up and revisit the code, gimme a little time and I'll get to it. |
|
@francoisfrisch thanks there is a new test stack on master. |
|
you may want to check mergev2 branch (#189) that a work in progress that remove listen in favor of observe. it will be for v6.0.0 release. in the mean time feel free to rebase on master. but that may cause an issue for v6. |
|
@hthetiot Sincerest apologies, but the project I had this use case for is long defunct and I've really not been using your library for anything else lately, nor do I have the extra time to devote to this (however minor it is). I'm just letting you know that I'm dropping off this thread and plan to delete my fork of your repo for now. You're welcome to use my code / changes if you want them, but you'll have to finish it yourself. Sorry! PS - does anyone know if I delete my fork will it mess up this pull request? I can leave it for a while if I need to / you want me to. |
In regards to issue #121:
There are times when listeners might want to make changes that trigger
further events in a collection. I had a bug in my code around this
point and only realized that further collection changes are suppressed
by default. This patch adds an override flag to allow for descriptors
to fire their listeners again even if currently active.
Signed-off-by: Fred Kilbourn [email protected]