search

reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

this.listenables on ES6 Store does not listen to child actions #478

Closed semikolon closed 8 years ago

semikolon commented 8 years ago

So, this.listenables is presented as a shortcut to this.listenToMany() in the readme/documentation

but I found when using Reflux.Store that they're not equivalent. The problem seems to be here: https://github.com/reflux/refluxjs/blob/master/dist/reflux.js#L1390

Why not just use listenToMany directly instead of an implementation of its own like the current code (above)?

devinivy commented 8 years ago

@BryanGrezeszak any thoughts on this?

BryanGrezeszak commented 8 years ago

Can you tell me in what exact ways you are noticing differences between what is expected and what exactly acts different than expected when you use this.listenables? I'd like to look into it, but it's hard to look into a bug without that exact info about it.

semikolon commented 8 years ago

The child actions aren't being listened to. Can give you code later, but: I make an action getAll and specify asyncResult: true, then make a listener getAll and getAllCompleted + ...Failed, in the store. But the completed and failed children actions do not ever get called, if using this.listenables, but they do when I use .listenToMany, again this is the callbacks in the store we're talking about On Sun, 7 Aug 2016 at 18:52, Bryan Grezeszak notifications@github.com wrote:

Can you tell me in what exact ways you are noticing differences between what is expected and what is seen? I'd like to look into it, but it's hard to look into a bug without that exact info about it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/reflux/refluxjs/issues/478#issuecomment-238093554, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2MP0jygDUcf6sE0p28cBedtv-sO2Yks5qdg1pgaJpZM4JeAX_ .

BryanGrezeszak commented 8 years ago

Thanks, that gives me a better idea of what's going on. I'll wait for your example code and use it as the main test case for the issue being solved and get it done.

BryanGrezeszak commented 8 years ago

@semikolon - I replicated it and messed with it and you're right on all counts, including the solution. It's one of those situations where you look back in your code and wonder "why didn't I do it that way in the first place?". It's a much better way of going about it. Thanks for finding it, isolating it, and bringing it to our attention.

I'll push soon along with some other things I'm working on.

semikolon commented 8 years ago

Hehe :-) Thanks! Happy to have contributed... I was about to write a failing regression test but never got to it. Anyway, good stuff. I'm intensely using Reflux now, consulting for a startup, so I really appreciate any attention payed to issues that pop up. On the whole it's awesome though!

On Tue, Aug 9, 2016 at 9:56 PM Bryan Grezeszak notifications@github.com wrote:

@semikolon https://github.com/semikolon - I replicated it and messed with it and you're right on all counts, including the solution. It's one of those situations where you look back in your code and wonder "why didn't I do it that way in the first place?". It's a much better way of going about it. Thanks for finding it, isolating it, and bringing it to our attention.

I'll push soon along with some other things I'm working on.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/reflux/refluxjs/issues/478#issuecomment-238671502, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2MLwLYJl0Tw9SrOGWA60rQRIc80YWks5qeNtTgaJpZM4JeAX_ .

devinivy commented 8 years ago

Good work, yall! Once this is in I'll make a proper release.

semikolon commented 8 years ago

Great! Could you also do a release of reflux-promise? I forked it to merge two different features from the network, so check that out. Kinda off topic, sorry On Tue, 9 Aug 2016 at 22:46, devin ivy notifications@github.com wrote:

Good work, yall! Once this is in I'll make a proper release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/reflux/refluxjs/issues/478#issuecomment-238685823, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2MONBqgXg-5zjQLipEMgc6sK89qNSks5qeOdBgaJpZM4JeAX_ .

BryanGrezeszak commented 8 years ago

I actually had some additions I was going to make. But since you're gonna make it a proper release I'll push it with just bugfixes and you can make a release. Then I'll add those new features later.

BryanGrezeszak commented 8 years ago

@devinivy - pull request is up with this change

devinivy commented 8 years ago

Great! Closed by #479.