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

Set active properties before emitting events #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sakeran
Copy link
Contributor

@Sakeran Sakeran commented Sep 25, 2019

This changes Effect#activate and Effect#deactivate such that the Effect's active property is set to the proper value before their corresponding events are emitted. Most importantly, this prevents a case where an infinite loop might be created.

Example (some debugEffect.js):

module.exports = {
  config: {
    duration: 1000
  },
  listeners: state => ({
    effectDeactivated: function() {
      this.target.emit('badNews'); // infinite loop
      Broadcast.sayAt(this.target, "Effect deactivated.");
    }
  })
};

In this case, EffectList#validateEffects and the listener function will be stuck invoking one another until the stack overflows. While less likely to occur practice, we could engineer a similar loop with Effect#activate and its related event.

Setting the active property before any events are called ensures that each method will short-circuit before this becomes an issue.

@nelsonsbrian
Copy link
Contributor

I think I have experienced this as well. I have had to gate the deactivate listener to only fire once in a few things. But never looked into it.

@azigler
Copy link

azigler commented Jun 7, 2020

Really nice catch!

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

Successfully merging this pull request may close these issues.

3 participants