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

Dispatch overridden w/ side effects when child emits new program event. #365

Open
steve-the-edwards opened this issue Jun 30, 2015 · 7 comments
Labels

Comments

@steve-the-edwards
Copy link

If an event (event alpha) is currently being processed (i.e. the Breath First Search is ongoing) for a particular program event via Dispatch, and a child node's (node X) handling of this event creates a new program event (event beta) targeted at the path of its own node, then this event will 'take over' the event Dispatch, but keep the remaining children in the queue. This results in event beta being sent to the neighbours of node X and not simply its children.

The following JSBin demonstrates this problem: https://jsbin.com/toroqaheze/edit (needs local images, but effect can still be seen).

To reproduce:
1,2,3,4 will change the speed of spinning of each child, but 3 will emit a new event (.hide()) as well, this overrides the currently processed event (speedChange) and the 4th node's speed is not changed.

Gist is here: https://gist.github.com/anonymous/e9f45883faad8540ff66

@steve-the-edwards
Copy link
Author

@DnMllr we were discussing on slack last week?

@michaelobriena
Copy link
Member

@DnMllr Can you comment?

@alexanderGugel
Copy link
Member

This is correct. The Dispatch doesn't handle this case correctly (emitting events while other events are being processed).

Sorry that is took that long. I made a quick fix for this. The problem basically is that the queue in Dispatch is being for multiple events. I changed it so it now creates a new one.

@steve-the-edwards
Copy link
Author

Any update on when this fix will be mainlined?

@jd-carroll
Copy link
Contributor

If a change is going to be made, I think it needs to also address:

  • Include the dispatching node as the first node to have 'onReceive' called
  • 'onReceive' should be able to return a payload which includes a 'stopPropagation' that will prevent any of that node's children from being added to the queue

However, from a pure technical standpoint, everything looks good.

And of course, this PR may not be the place for such a change.

@steve-the-edwards
Copy link
Author

@jd-carroll the first point is covered here: #366.

alexanderGugel added a commit to alexanderGugel/engine that referenced this issue Jul 31, 2015
@jd-carroll
Copy link
Contributor

This issue can be closed as a fix has been landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants