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

Share EventEmitter among group of related actions. #13

Closed
wants to merge 3 commits into from

Conversation

dashed
Copy link
Contributor

@dashed dashed commented Jul 29, 2014

Work in progress


  • Update readme

Reflux.createActions creates an instance of _.EventEmitter for every element in a given array of unique action names. Such an array of action names should be assumed to share 'context' or namespace.

Added check if an action has already been created for the edge case of duplicate action names.

@dashed
Copy link
Contributor Author

dashed commented Jul 29, 2014

Unless Reflux.createActions is meant to be a utility function to create many independent actions; I think it's better to infer that it creates 'namespaced' actions.

An _.EventEmitter instance for every action is a waste.

@spoike
Copy link
Member

spoike commented Jul 29, 2014

If you have time, please do add a test spec that tests this in the test folder.

@spoike spoike mentioned this pull request Jul 29, 2014
@spoike spoike added this to the 0.1.3 milestone Jul 29, 2014
@dashed
Copy link
Contributor Author

dashed commented Jul 29, 2014

Added some tests.


I think allowing to name actions is a mistake; otherwise there will be collisions when dynamically shuffling actions around.

reflux should internally generate UUIDs for actions, and users should just focus on passing functions by reference.

@dashed
Copy link
Contributor Author

dashed commented Jul 29, 2014

Removed the need to generate unique ids within Reflux.createActions(); so it's left up to the developer to give an array of unique action names which is sufficient enough.

The object returned by Reflux.createActions() should be assumed to be immutable (pretend Object.freeze()); this is due to the fact that you may no longer add/remove actions to/from a shared EventEmitter, as per #14.

beforeEach(function () {

context = new Namespace();
actionContext = Reflux.createAction(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be easier to let the namespace/context take care of action creation instead (since ´Namespace´ doesn't extend EventEmitter that much to the point that it is quite anemic):

context = new Namespace();
actionOnContext = context.createAction();

That way Reflux.createAction() will instead create an action from a default context found on Reflux.defaultContext or something.

@spoike
Copy link
Member

spoike commented Jul 29, 2014

Still on the fence with deciding if it should be added or not. I'm wondering if this isn't just another case of premature optimization, we need to do some performance/memory comparisons together with the current solution to convince me that this is needed.

@spoike
Copy link
Member

spoike commented Jul 29, 2014

Also this also disregards the fact that stores also uses the EventEmitter. So we probably need to do the same thing with them.

@dashed
Copy link
Contributor Author

dashed commented Jul 29, 2014

I don't think this is premature optimization. If you're only registering callbacks to listen only to a single event within EventEmitter (i.e. an action), then EventEmitter is already overkill.

In practice, you usually have many actions grouped together forming a concern, so I think it makes sense if they share an EventEmitter. Each action would be assigned as an 'event' within an associated EventEmitter.


I think an EventEmitter per store is sufficient, since one store usually listen to multiple actions. I don't think there is a need to do "grouping" at this part of the architecture.

@bripkens
Copy link
Contributor

I am still wondering what the goal of theses changes is. Performance and memory usage might be a concern, but neither Node.js' native EventEmitter nor EventEmitter3 seem problematic. Especially considering that Actions and Stores are most likely to be created upon application bootstrap.

Is there any real world scenario where you might want to observe a group of actions? If so, is the concept of Namespaces sufficient for this or might an application need to define groups of actions in an ad-hoc way? Thinking about something like:

var group = Reflux.createActionGroup(changeSettingsAction, activePageAction);

group.listen(console.log, console);

@spoike spoike removed this from the 0.1.3 milestone Jul 29, 2014
@spoike
Copy link
Member

spoike commented Jul 29, 2014

I'm leaving this PR out of 0.1.3 milestone and into limbo now to think about it for a while. I can see where @dashed was initially going with this, but I have trouble finding real value for it. Micromanaging how the EventHandler is used inside the actions in your application seems to just add more verbosity to our current API than what it's worth and the tests written in this PR isn't convincing me to merge it in.

I'm open for suggestions on how to create extension points in Reflux for this if anyone wants to monkey patch that kind of functionality in their own project.


In the project that I'm dogfooding Reflux with, I found no need to handle actions this way, and we currently have almost 10 of them with no performance or memory problems. It runs swimmingly even when one of them is invoked like crazy from mouse and touch events (even in the Android Chrome browser) and performance bottle necks are usually found in other parts of the application.

We also have a user story that is adding/removing listeners like crazy initiated from mouse and touch events against a data store for performance reasons and there are no problems there either.

The only thing we needed to do to avoid thrashing issues from events was to throttle them, which is a common optimization for these kinds of issues.

@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

Perhaps we can revisit this in the future. I'll close this PR for now.

@dashed dashed closed this Jul 30, 2014
@spoike spoike mentioned this pull request Jul 30, 2014
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