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

allowing preEmit to change arguments #78

Merged
merged 7 commits into from Sep 23, 2014

Conversation

Projects
None yet
4 participants
@krawaller
Contributor

krawaller commented Sep 18, 2014

This PR implements the previously discussed functionality that if preEmit returns an array, that should be used as arguments instead when passed on to shouldEmit and (potentially) trigger.

I considered allowing returning non-undefined non-array values too and just wrapping them in an array, but then things get somewhat unintuitive if what you actually want is a single parameter that is an array.

Always returning an array might be clunky when you only want one parameter, but it means less space for misunderstanding.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 18, 2014

Member

I appreciate the work. I'd imagine three cases of preEmit's:

  • Returns nothing (i.e. undefined)
action.preEmit = function() {
   dosomething();
};
// Will use original arguments
  • Returns an array or arraylike:
action.preEmit = function() {
    arguments[0] = "lawl - always lawl";
    return arguments;
};
// Will apply the array or arraylike as arguments to the listeners
  • Returns a single value or object:
action.preEmit = function() {
    var sum = _.reduce(arguments, function(m, arg) {return m+arg;}, 0);
    return sum;
};
// Will call the listener with the one value as argument
Member

spoike commented Sep 18, 2014

I appreciate the work. I'd imagine three cases of preEmit's:

  • Returns nothing (i.e. undefined)
action.preEmit = function() {
   dosomething();
};
// Will use original arguments
  • Returns an array or arraylike:
action.preEmit = function() {
    arguments[0] = "lawl - always lawl";
    return arguments;
};
// Will apply the array or arraylike as arguments to the listeners
  • Returns a single value or object:
action.preEmit = function() {
    var sum = _.reduce(arguments, function(m, arg) {return m+arg;}, 0);
    return sum;
};
// Will call the listener with the one value as argument
@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 18, 2014

Contributor

That's the approach I considered and discarded, fearing it would be confusing. But you're right (again), it does feel rather intuitive!

Contributor

krawaller commented Sep 18, 2014

That's the approach I considered and discarded, fearing it would be confusing. But you're right (again), it does feel rather intuitive!

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 18, 2014

Contributor

Code and tests now match your three use cases.

Contributor

krawaller commented Sep 18, 2014

Code and tests now match your three use cases.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 18, 2014

Contributor

Hmm - this breaks when we return the arguments array-like. Fixing.

Contributor

krawaller commented Sep 18, 2014

Hmm - this breaks when we return the arguments array-like. Fixing.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 18, 2014

Contributor

Ok, works now. Ended up having to make a special case for when preEmit returns the arguments arraylike, but that shouldn't be too expensive.

Contributor

krawaller commented Sep 18, 2014

Ok, works now. Ended up having to make a special case for when preEmit returns the arguments arraylike, but that shouldn't be too expensive.

@krawaller krawaller referenced this pull request Sep 23, 2014

Merged

0.1.8 #67

assert.deepEqual(args[1],[newarg]);
});
});
describe("when preEmit returns a function",function(){

This comment has been minimized.

@spoike

spoike Sep 23, 2014

Member

weird whitespace issue here. Please use editorconfig plugin in the future for your IDE or text editor to avoid this, as we have a .editorconfig file for these kinds of issues.

@spoike

spoike Sep 23, 2014

Member

weird whitespace issue here. Please use editorconfig plugin in the future for your IDE or text editor to avoid this, as we have a .editorconfig file for these kinds of issues.

spoike added a commit that referenced this pull request Sep 23, 2014

Merge pull request #78 from krawaller/preemitargs
allowing preEmit to change arguments

@spoike spoike merged commit 703e4bb into reflux:0.1.8 Sep 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 23, 2014

Member

Merging this anyway. Will go over whitespace issues later.

Member

spoike commented Sep 23, 2014

Merging this anyway. Will go over whitespace issues later.

@spoike spoike added this to the 0.1.8 milestone Sep 24, 2014

@krawaller krawaller deleted the krawaller:preemitargs branch Oct 2, 2014

@@ -103,7 +103,7 @@ Actions.statusUpdate();
There are a couple of hooks avaiable for each action.
* `preEmit` - Is called before the action emits an event. It receives the arguments from the action invocation.
* `preEmit` - Is called before the action emits an event. It receives the arguments from the action invocation. If it returns something other than undefined, that will be used as arguments for `shouldEmit` and subsequent emission.

This comment has been minimized.

@Strajk

Strajk Apr 21, 2015

@krawaller Hi there, I'm trying to grasp this feature... can you clarify subsequent emission please?

@Strajk

Strajk Apr 21, 2015

@krawaller Hi there, I'm trying to grasp this feature... can you clarify subsequent emission please?

This comment has been minimized.

@jankuca

jankuca Apr 21, 2015

@Strajk: It just says that whatever you return from #preEmit() gets passed to #shouldEmit() and the registered listeners (given #shouldEmit() does not return false).

Something along these lines happens internally:

var modifiedArgs = pub.preEmit(...args);
if (typeof modifiedArgs === 'undefined') {
  modifiedArgs = args;
}
if (pub.shouldEmit(...modifiedArgs)) {
  // this is the "subsequent emission"
  listeners.forEach(listener => listener(...modifiedArgs));
}
@jankuca

jankuca Apr 21, 2015

@Strajk: It just says that whatever you return from #preEmit() gets passed to #shouldEmit() and the registered listeners (given #shouldEmit() does not return false).

Something along these lines happens internally:

var modifiedArgs = pub.preEmit(...args);
if (typeof modifiedArgs === 'undefined') {
  modifiedArgs = args;
}
if (pub.shouldEmit(...modifiedArgs)) {
  // this is the "subsequent emission"
  listeners.forEach(listener => listener(...modifiedArgs));
}

This comment has been minimized.

@Strajk
@Strajk
@Strajk

This comment has been minimized.

Show comment
Hide comment
@Strajk

Strajk Apr 21, 2015

Hi there, I'm trying to fully understand Reflux and I was wondering what are some valid and also best practice usage of this feature? Can you give me some examples when it's good to manipulate data in preEmit?

@krawaller @spoike

Strajk commented Apr 21, 2015

Hi there, I'm trying to fully understand Reflux and I was wondering what are some valid and also best practice usage of this feature? Can you give me some examples when it's good to manipulate data in preEmit?

@krawaller @spoike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment